stylo: Fix sharing of visited style contexts

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
In bug 1367904 we start using the servo-side visited styles.

However, if styles are being shared, this may mark unvisited styles being shared (e.g. siblings) as visited too.

We were failing some mochitests because of this and marked them as expected.



FAIL | layout/style/test/test_visited_reftests.html | reftest comparison: == selector-descendant-2.xhtml selector-descendant-2-ref.xhtml [log…]

 
FAIL | layout/style/test/test_visited_reftests.html | reftest comparison: == subject-of-selector-descendant-2.xhtml subject-of-selector-descendant-2-ref.xhtml [log…]



We should either Arc::make_mut these when setting visited styles, or:

14:23 < emilio> bholley: if so, fine for me... You can also just change the same_state_ignoring_visitedness to not ignore visitedness,
                I guess
Not ignoring visitedness seems like the right solution to me, fwiw.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8887594 - Flags: review?(bzbarsky)
Comment hidden (mozreview-request)
Comment on attachment 8887594 [details]
Bug 1381635 - stylo: Don't ignore visited state when deciding to share style contexts;

https://reviewboard.mozilla.org/r/158460/#review163710

::: servo/components/style/sharing/mod.rs:646
(Diff revision 2)
>          if target.matches_user_and_author_rules() !=
>              candidate.element.matches_user_and_author_rules() {
>              miss!(UserAndAuthorRules)
>          }
>  
> -        if !checks::have_same_state_ignoring_visitedness(target.element, candidate) {
> +        if target.element.get_state() != candidate.get_state() {

This should have a comment about why we're including visitedness state, because it's definitely not obvious.
Attachment #8887594 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)

Comment 7

11 months ago
mozreview-review
Comment on attachment 8887594 [details]
Bug 1381635 - stylo: Don't ignore visited state when deciding to share style contexts;

https://reviewboard.mozilla.org/r/158460/#review163760

::: layout/style/test/stylo-failures.md:70
(Diff revision 3)
>  * test_author_specified_style.html: support serializing color as author specified bug 1348165 [27]
>  * browser_newtab_share_rule_processors.js: agent style sheet sharing [1]
>  * :visited support (bug 1328509)
> -  * test_visited_reftests.html `inherit-keyword-1.xhtml` [2]
> -  * test_visited_reftests.html `selector-descendant-2.xhtml`: bug 1381635 [4]
>    * ... `mathml-links.html` [2]

This line should now list the visited test file, instead of just "...".
Maybe this fixes the mathml-links.html one as well?
(Assignee)

Comment 9

11 months ago
IIRC it doesn't, the mathml failure was always there
(Assignee)

Comment 10

11 months ago
https://github.com/servo/servo/pull/17778



https://hg.mozilla.org/mozilla-central/rev/d396287f2595
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
No longer blocks: 1377469

Comment 11

11 months ago
2 failures in 822 pushes (0.002 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2

Platform breakdown:
* linux64-stylo: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1381635&startday=2017-07-17&endday=2017-07-23&tree=all

Updated

11 months ago
status-firefox56: --- → fixed
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.