Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When an iframe loads a new page, dirty and reflow it so that the layout fragment will pick up the new subpage ID. #8662

Closed

Conversation

pcwalton
Copy link
Contributor

This is necessary to make its new contents show up!

Closes #8081.

r? @jdm

cc @paulrouget @glennw

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 24, 2015
@pcwalton
Copy link
Contributor Author

cc @metajack as well.

@eefriedman
Copy link
Contributor

The page shouldn't actually be changing visibly until around step 22 of the navigate algorithm, after we see the initial response from the server (https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents). This patch marks the iframe dirty and forces a reflow around step 10 or so.

I'm not completely sure whether that indicates a problem with your patch or a more general architectural issue.

@pcwalton
Copy link
Contributor Author

I think the correct solution to that is to not update the iframe DOM node's stored subpage ID until the right time. Relying on the timing of dirty bits and reflows is wrong since those can be set or can happen for lots of reasons.

In other words I think that's a preexisting problem independent of this patch.

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 24, 2015

Why is the force_reflow necessary exactly? It would be nice to have documentation so I could figure out when it's necessary from first principles rather than having to take it on faith.

@pcwalton
Copy link
Contributor Author

I'll see if I can use reflow instead of force reflow.

Renaming force reflow and reflow seems out of scope for this patch.

fragment will pick up the new subpage ID.

This is necessary to make its new contents show up!

Closes servo#8081.
@pcwalton
Copy link
Contributor Author

Updated to use reflow instead of force_reflow. r? @jdm

@pcwalton pcwalton force-pushed the iframe-painting-after-navigation branch from e5f5d90 to 3cafde1 Compare November 24, 2015 21:06
@jdm
Copy link
Member

jdm commented Dec 1, 2015

@eefriedman is right. I think this actually intersects with #8612, and we should piggy-back off of that work. I propose we add a Constellation->Script message called FramedContentChanged, which is sent to the script task for the framing pipeline. This will mark the iframe element as dirty and initiate the reflow, and will be sent from the code in the constellation that receives the ActivateDocument message in #8612.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 1, 2015
@pcwalton
Copy link
Contributor Author

#8612 seems blocked for whatever reason. Status?

@jdm
Copy link
Member

jdm commented Dec 16, 2015

@glennw and I decided to go with his proposed simple solution in that PR last week.

@glennw
Copy link
Member

glennw commented Dec 16, 2015

I implemented the simple solution yesterday. But when I ran WPT tests a heap of unexpected failures occurred. I suspect it's exposing a different race, but perhaps there is a bug in the implementation. I'll try to look into it again today.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #8618) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Dec 17, 2015
@glennw
Copy link
Member

glennw commented Jan 4, 2016

@pcwalton @jdm #8612 has landed now, if that was still blocking further work here.

@metajack
Copy link
Contributor

@pcwalton This is the last known P1 browser.html blocker, so let's get this rebased an landed.

@pcwalton
Copy link
Contributor Author

Superseded by #9285.

@pcwalton pcwalton closed this Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants