layout: Produce an empty display list when the Document element is removed#44133
Merged
Conversation
Contributor
Author
|
@mukilan I think this fixes iframe content not being removed! |
Member
|
@rovertrack A couple things:
|
Contributor
Author
Thanks @mrobinson, will add tests for this fix |
|
🔨 Triggering try run (#24301470301) for Linux (WPT) |
|
Test results for linux-wpt from try job (#24301470301): Flaky unexpected result (35)
Stable unexpected results that are known to be intermittent (22)
Stable unexpected results (2)
|
|
|
mrobinson
requested changes
Apr 12, 2026
Comment on lines
+698
to
+703
| Some(element) => element, | ||
| None => return, | ||
| None => { | ||
| // Trigger update if the document element was removed. | ||
| self.add_restyle_reason(RestyleReason::DOMChanged); | ||
| return; | ||
| }, |
Member
There was a problem hiding this comment.
It looks like this will constantly add restyles if there is no document, but we probably only want to do this once when the document is removed.
Contributor
Author
There was a problem hiding this comment.
Added root_removal_noted flag for better tracking and to make sure this is only done once!
Comment on lines
+874
to
+887
| // If we previously generated a display list, clear cached trees and | ||
| // send an empty transaction to clear the stale content. | ||
| if self.have_ever_generated_display_list.get() { | ||
| self.box_tree.borrow_mut().take(); | ||
| self.fragment_tree.borrow_mut().take(); | ||
| self.stacking_context_tree.borrow_mut().take(); | ||
| self.paint_api | ||
| .send_initial_transaction(self.webview_id, self.id.into()); | ||
| self.have_ever_generated_display_list.set(false); | ||
| return Some(ReflowResult { | ||
| reflow_phases_run: ReflowPhasesRun::BuiltDisplayList, | ||
| ..Default::default() | ||
| }); | ||
| } |
Member
There was a problem hiding this comment.
We likely only want to do this once when the document becomes empty. We should probably track whether the last display we sent was empty and also move this to a helper method called send_empty_display_list. Additionally, I don't think we want to reuse send_initial_transaction here as that has a special behavior.
Contributor
Author
There was a problem hiding this comment.
in the latest changes
- Added a last_display_list_was_empty flag for tracking
- Implemented the send_empty_display_list helper method
fc7d6a0 to
1a3df8d
Compare
Contributor
Author
|
@mrobinson all the suggested improvements have been made test added |
mrobinson
requested changes
Apr 15, 2026
Comment on lines
+1312
to
+1313
| self.last_display_list_was_empty.set(false); | ||
|
|
Member
There was a problem hiding this comment.
Let's set this below where we set the other LayoutImpl flags.
Contributor
Author
There was a problem hiding this comment.
Done in latest changes.
| pending_scroll_events: Default::default(), | ||
| rendering_update_reasons: Default::default(), | ||
| waiting_on_canvas_image_updates: Cell::new(false), | ||
| root_removal_noted: Cell::new(false), |
Member
There was a problem hiding this comment.
I think this needs to be true as there we don't need to send an empty display list when the page is first created.
Contributor
Author
There was a problem hiding this comment.
Done in latest changes.
Comment on lines
+700
to
+712
| let document_element = match self.GetDocumentElement() { | ||
| Some(element) => element, | ||
| None => return, | ||
| Some(element) => { | ||
| self.root_removal_noted.set(false); | ||
| element | ||
| }, | ||
| None => { | ||
| // Trigger update if the document element was removed. | ||
| if !self.root_removal_noted.get() { | ||
| self.add_restyle_reason(RestyleReason::DOMChanged); | ||
| self.root_removal_noted.set(true); | ||
| } | ||
| return; | ||
| }, |
Member
There was a problem hiding this comment.
This can just be:
let Some(document_element) = self.GetDocumentElement() else {
// Trigger update if the document element was removed.
if !self.root_removal_noted.get() {
self.add_restyle_reason(RestyleReason::DOMChanged);
self.root_removal_noted.set(true);
}
return;
};
// This ensures that if the document element is removed in the future, it
// will trigger a new empty display list.
self.root_removal_noted(false);
Contributor
Author
There was a problem hiding this comment.
Done in latest changes.
Comment on lines
+1387
to
+1389
| /// Clear all cached layout trees | ||
| /// Send empty display list to inform the compositor to clear its display list. | ||
| fn send_empty_display_list(&self) -> Option<ReflowResult> { |
Member
There was a problem hiding this comment.
Suggested change
| /// Clear all cached layout trees | |
| /// Send empty display list to inform the compositor to clear its display list. | |
| fn send_empty_display_list(&self) -> Option<ReflowResult> { | |
| /// Clear all cached layout trees and send an empty display list to paint. | |
| fn clear_layout_trees_and_send_empty_display_list(&self) -> Option<ReflowResult> { |
Comment on lines
+1390
to
+1407
| let paint_info = self | ||
| .stacking_context_tree | ||
| .borrow() | ||
| .as_ref() | ||
| .map(|tree| tree.paint_info.clone())?; | ||
| self.box_tree.borrow_mut().take(); | ||
| self.fragment_tree.borrow_mut().take(); | ||
| self.stacking_context_tree.borrow_mut().take(); | ||
| let mut builder = webrender_api::DisplayListBuilder::new(paint_info.pipeline_id); | ||
| builder.begin(); | ||
| let (_, empty_display_list) = builder.end(); | ||
| self.paint_api | ||
| .send_display_list(self.webview_id, &paint_info, empty_display_list); | ||
| self.last_display_list_was_empty.set(true); | ||
| Some(ReflowResult { | ||
| reflow_phases_run: ReflowPhasesRun::BuiltDisplayList, | ||
| ..Default::default() | ||
| }) |
Member
There was a problem hiding this comment.
Let's no interlace logic for one step with another. You also should create a new paint info instead of using the old one:
Suggested change
| let paint_info = self | |
| .stacking_context_tree | |
| .borrow() | |
| .as_ref() | |
| .map(|tree| tree.paint_info.clone())?; | |
| self.box_tree.borrow_mut().take(); | |
| self.fragment_tree.borrow_mut().take(); | |
| self.stacking_context_tree.borrow_mut().take(); | |
| let mut builder = webrender_api::DisplayListBuilder::new(paint_info.pipeline_id); | |
| builder.begin(); | |
| let (_, empty_display_list) = builder.end(); | |
| self.paint_api | |
| .send_display_list(self.webview_id, &paint_info, empty_display_list); | |
| self.last_display_list_was_empty.set(true); | |
| Some(ReflowResult { | |
| reflow_phases_run: ReflowPhasesRun::BuiltDisplayList, | |
| ..Default::default() | |
| }) | |
| // Clear layout trees. | |
| self.box_tree.borrow_mut().take(); | |
| self.fragment_tree.borrow_mut().take(); | |
| self.stacking_context_tree.borrow_mut().take(); | |
| // Send empty display list. | |
| let paint_info = PaintDisplayListInfo::new( | |
| viewport_details, | |
| Size2D::zero(), | |
| self.id.into(), | |
| reflow_request.epoch, | |
| AxesScrollSensitivity { x: ScrollType::InputEvents | ScrollType::Script, y: ScrollType::InputEvents | ScrollType::Script }, | |
| !self.have_ever_generated_display_list.get(), | |
| ); | |
| let mut builder = webrender_api::DisplayListBuilder::new(paint_info.pipeline_id); | |
| builder.begin(); | |
| let (_, empty_display_list) = builder.end(); | |
| self.paint_api | |
| .send_display_list(self.webview_id, &paint_info, empty_display_list); | |
| self.last_display_list_was_empty.set(true); | |
| self.set_have_ever_generated_display_list.set(true); | |
| Some(ReflowResult { | |
| reflow_phases_run: ReflowPhasesRun::BuiltDisplayList, | |
| ..Default::default() | |
| }) |
Contributor
Author
There was a problem hiding this comment.
I didn't like the idea of reallocating a separate paint api guess we still should do it anyways 😅
Member
There was a problem hiding this comment.
The epoch of the previous one is incorrect and I don't know if we can absolutely guarantee that a previous one exists. Consider the case where you immediately remove the Document element before the pipeline has had a chance to create the stacking context tree. This kind of assumption makes me nervous and I think we should write the code defensively here. In the end, the entire data structure is serialized or copied onto the channel anyway.
Contributor
Author
There was a problem hiding this comment.
Great 👍 will implement it
Thanks
Member
|
There are two new subtests in @rovertrack: You'll need to update the test expectations for these two subtests. It is documented in the book here. |
Member
@mrobinson : I think you actually added the previous |
Contributor
Author
@mukilan so was this the reason this is not merged ?? |
Contributor
|
Yes |
Signed-off-by: Rover track <rishan.pgowda@gmail.com>
Signed-off-by: Rover track <rishan.pgowda@gmail.com>
Collaborator
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#59312). |
Member
Oh, you are probably right @mukilan. Sorry, @rovertrack, it was my fault all along. 🤦 |
Contributor
Author
@mrobinson please that happens to all of us ,i have messed up things more than you in this PR 😅 |
Contributor
Author
|
@mukilan thanks for the instructions I have updated the tests regarding the expectation of the subtests which you had referred |
Collaborator
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#59312) title and body. |
Contributor
Author
|
Thanks to everyone for their support on this PR 🙂! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, when the
Documentelement was removed, no further display list updates would be sent to paint. This change makes it so that when theDocumentelement is removed a single new empty display list is sent.Testing: This change adds a new WPT test .
Fixes: #44101