Skip to content

/ servo

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

Clears #5909 #7302

Closed
wants to merge 2 commits into from
Closed

Clears #5909 #7302

wants to merge 2 commits into from

Conversation

@Adenilson
Copy link
Contributor

Adenilson commented Aug 21, 2015

Recent changes on Fragment::establishes_stacking_context() makes this hack no longer necessary for displaying filtered inline elements.

Review on Reviewable
@highfive
Copy link

highfive commented Aug 21, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@Adenilson Adenilson mentioned this pull request Aug 21, 2015
Closed
components/layout/display_list_builder.rs Outdated
}
};
match self.fragments.fragments[0].specific {
SpecificFragmentInfo::Canvas(_) => has_stacking_context = has_stacking_context && true,

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Aug 21, 2015

Contributor

&& true?

This comment has been minimized.

Sign in to view
Copy link
@Adenilson

Adenilson Aug 21, 2015

Author Contributor

Looks to be the case for "|| true" or simple set has_stacking_context = true;

I'm assuming that we need a stacking context when there is Canvas?
@pcwalton
Copy link
Contributor

pcwalton commented Aug 21, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2015

📌 Commit e000b74 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2015

⌛ Testing commit e000b74 with merge 7233cc3...
bors-servo pushed a commit that referenced this pull request Aug 21, 2015
bors-servo
Auto merge of #7302 - Adenilson:removeCreationSC4FilteredElement, r=p…
7233cc3 
…cwalton

Clears #5909

Recent changes on Fragment::establishes_stacking_context() makes this hack no longer necessary for displaying filtered inline elements.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7302)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2015

💔 Test failed - linux1
@jdm
Copy link
Member

jdm commented Aug 22, 2015 


failures:
    acid1_a.html == acid1_b.html
    iframe/stacking_context.html == iframe/stacking_context_ref.html
    inline_block_stacking_context_a.html == inline_block_stacking_context_ref.html
@Adenilson
Recent changes on Fragment::establishes_stacking_context() makes this
Loading status checks…
508979e 
hack no longer necessary for displaying filtered inline elements.

Interestingly, Acid1 will panic with index out of bounds. Now we test for
Fragments vector length before reading it.
@Adenilson Adenilson force-pushed the Adenilson:removeCreationSC4FilteredElement branch from e000b74 to 508979e Aug 24, 2015
@Adenilson
Copy link
Contributor Author

Adenilson commented Aug 24, 2015

I was able to reproduce the error in OSX, more info: https://gist.github.com/Adenilson/4eb3ab62247ffbc625b8

I applied a check for the Fragments vector length and it stops the panic from happening.
@Adenilson
Copy link
Contributor Author

Adenilson commented Aug 24, 2015

That solves the case for acid1, I'm investigating what is wrong with the other 2 tests.
@Adenilson
Canvas fragment info is already checked on Fragment::establishes_stac…
Loading status checks…
7f2645a 
…king_context().
@Adenilson
Copy link
Contributor Author

Adenilson commented Aug 25, 2015

Did some digging, the failing tests have associated SC (Stacking Context) but won't draw correctly. It seems that the original 'stacking_context && something_something' was required to turn to false the stacking_context flag.

Otherwise the element won't be displayed. It is a bit unclear if that is thanks to Paint code or past DL (Display List) optimization.

I also noticed that there is no need for checking the Canvas fragment presence, as it is already done as a special case in Fragment::establishes_stacking_context().

The current patch removes this unnecessary check and documents this issue. I executed all the 354 ref tests to make sure that there is no regression.
@Adenilson
Copy link
Contributor Author

Adenilson commented Sep 1, 2015

Anyone could kick the bots on this one?
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 2, 2015

r? @pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2015

☔ The latest upstream changes (presumably #7423) made this pull request unmergeable. Please resolve the merge conflicts.
@pcwalton
Copy link
Contributor

pcwalton commented Nov 2, 2015

@mrobinson Is this fixed now with your stuff?
@mrobinson
Copy link
Member

mrobinson commented Nov 2, 2015

@pcwalton I think that this requires a further fix, which is to also integrate stacking contexts into the list of inlines. I've started working on that today, though I think it will be a multi-stage process.
@frewsxcv
Copy link
Member

frewsxcv commented Dec 2, 2015

What is the status of this?
@mrobinson
Copy link
Member

mrobinson commented Dec 2, 2015

I think that #8376 makes this PR unnecessarily. It fixes the issue that I mention in my comment. Additionally, it turns out that inlines with stacking contexts belong in the list of positioned content to match what other browsers do.
@jdm jdm closed this Dec 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pcwalton pcwalton

Labels
S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@Adenilson @highfive @pcwalton @bors-servo @jdm @Ms2ger @mrobinson @frewsxcv
You can’t perform that action at this time.