Closed Bug 1381235 Opened 7 years ago Closed 7 years ago

stylo: issue with transitioning on visited links

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jandem, Assigned: chenpighead)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

STR:

(1) Enable stylo
(2) Load https://www.officeheart.nl/
(3) Hover over the Home button in the top-left-ish corner, a menu appears
(4) Click on a random menu item, say the first one
(5) Now open the same menu again, hover over each item, and notice that the background color is not "invalidated" correctly. I think this happens only for :visited items.
I suspect this may have to do with bug 1380133, and/or bug 1377975... Removing the transition makes the bug go away.
Priority: -- → P2
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> I suspect this may have to do with bug 1380133, and/or bug 1377975...

FWIW, I can still repro this on autoland tip so bug 1380133 didn't fix this one.
I can reproduce this on autoland tip on my mac as well, I'll try to make a reduced test case then.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Attached file simplified testcase
This is a reduced testcase that I further reduced from the original web site. Follow the STR then the issue could be easily reproduced:

1. click one of the link, say 'HOME', and jump to the link "https://www.officeheart.nl/"
2. press the back button to go back to the previous page, i.e., our test page
3. move mouse over the 'HOME' item
4. move mouse out of the 'HOME' item
5. repeat step 3 & 4, then an intermediate status of transition effect is shown


Looks like the :hover style is applied correctly, but the transitioning style on visited link is definitely applied wrongly. I suspect something with the cascading for transition/visited might be wrong.
Attachment #8889408 - Attachment is obsolete: true
Summary: stylo: issue with popup menu and visited (?) styles → stylo: issue with transitioning on visited links
yeah, in the animation inspector devtools it seems that the transition is running correctly with correct before and after style. But the transitioning style is never shown up visually.

A key point is here [1], if we drop the animation-only traversal check we can see the transition correctly. But I need to think it more detail since I don't quite understand how we handle visited rules.

[1] https://hg.mozilla.org/mozilla-central/file/131e19a573e9/servo/components/style/matching.rs#l650
Blocks: 1382742
At the moment, there are certain animation / transition code paths that haven't been updated for visited, so they might be related. (However, Gecko has some known issues with visited transitions as well, see bug 868975.)

:birtles and I discussed the various places we might need extra visited handling in bug 868975 comment 11.  Some of this code has moved around since then.  The `process_animations` code[1] doesn't currently check the visited style in functions like `needs_animations_update`, `needs_transition_update`, etc. leading to probably incorrect results.

[1]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/servo/components/style/matching.rs#196
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> At the moment, there are certain animation / transition code paths that
> haven't been updated for visited, so they might be related. (However, Gecko
> has some known issues with visited transitions as well, see bug 868975.)

Yeah, thanks for point this out. I followed your code comments and did a little archaeology in bug 868975 yesterday. Although Gecko has some known issues with visited transitions, and fixing those issues in Stylo might not be very high priority IMO, we still need to match Stylo's behavior with Gecko at the moment.

> :birtles and I discussed the various places we might need extra visited
> handling in bug 868975 comment 11.  Some of this code has moved around since
> then.  The `process_animations` code[1] doesn't currently check the visited
> style in functions like `needs_animations_update`,
> `needs_transition_update`, etc. leading to probably incorrect results.
> 
> [1]:
> http://searchfox.org/mozilla-central/rev/
> 3a3af33f513071ea829debdfbc628caebcdf6996/servo/components/style/matching.
> rs#196

Yes, that's the thing that I'd like to do in this bug. I suppose if we take visited styles update handling into consideration during transition/animation update, issues in this bug would be resolved and Stylo's behavior should match Gecko's.
Hmm that does not match what I am seeing in animation-inspector.  In the animation inspector, the transition has correct before and after-change style and runs correctly. But the problem is the visual stays a fixed color. So I think process_animations() works fine, but the transition style is not replaced during animation-only restyle.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> Hmm that does not match what I am seeing in animation-inspector.  In the
> animation inspector, the transition has correct before and after-change
> style and runs correctly. But the problem is the visual stays a fixed color.
> So I think process_animations() works fine, but the transition style is not
> replaced during animation-only restyle.

Yes, the styles in the animation inspector appear to be correct to me as well.
I am now confident we don't need to check animation-only traversal in replace_rules(), we can just call replace_rules_internal() for visited rules as well. (But I guess we can do a tiny optimization that checks we need visited rules there just like we do in cascade_style_and_visited [1]). Even in an animation-only restyle we try to get the visited rules in the cascade_style_and_visited() and use it in the cascade process.

So anyway, we should drop the check in replace_rules() I think.

[1] https://hg.mozilla.org/mozilla-central/file/9eddb0a92820/servo/components/style/style_resolver.rs#l213
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> I am now confident we don't need to check animation-only traversal in
> replace_rules(), we can just call replace_rules_internal() for visited rules
> as well. (But I guess we can do a tiny optimization that checks we need
> visited rules there just like we do in cascade_style_and_visited [1]). Even
> in an animation-only restyle we try to get the visited rules in the
> cascade_style_and_visited() and use it in the cascade process.
> 
> So anyway, we should drop the check in replace_rules() I think.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/9eddb0a92820/servo/components/
> style/style_resolver.rs#l213

Thanks for the help. I can confirm the issue disappeared if we drop the check. Without the check, the behavior of Stylo is exact the same as Gecko. I've pushed a try to see if this change breaks anything. If everything goes well, I'll request review then.
try w/ removing the check looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fc20a5414da7599f2fa7dec63beda546e16f82f

I'm still working on writing a test. In the meantime, I'd like to request review for the fix first, to make sure if I'm doing the right thing.
Attachment #8890256 - Flags: review?(hikezoe)
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review166682

Thanks for fixing this!
As you commented this definitely needs a test case.

::: commit-message-9eddb:3
(Diff revision 1)
> +Bug 1381235 - stylo: fix the optimization check in replace_rules() for visited styles.
> +
> +We skipped updating the rule nodes for visited rules during animation/transition.

during animation-only restyle.

::: commit-message-9eddb:4
(Diff revision 1)
> +Bug 1381235 - stylo: fix the optimization check in replace_rules() for visited styles.
> +
> +We skipped updating the rule nodes for visited rules during animation/transition.
> +The intent is to do some optimization for animation/transition. However, this causes

I think 'The intent ..' sentence is not neccessary.

::: commit-message-9eddb:5
(Diff revision 1)
> +Bug 1381235 - stylo: fix the optimization check in replace_rules() for visited styles.
> +
> +We skipped updating the rule nodes for visited rules during animation/transition.
> +The intent is to do some optimization for animation/transition. However, this causes
> +issues that prevent us from running animation/transition on visited elements.

isseus that visited style overries animation styles on visited element.

::: commit-message-9eddb:6
(Diff revision 1)
> +Bug 1381235 - stylo: fix the optimization check in replace_rules() for visited styles.
> +
> +We skipped updating the rule nodes for visited rules during animation/transition.
> +The intent is to do some optimization for animation/transition. However, this causes
> +issues that prevent us from running animation/transition on visited elements.
> +So, it turns out that we should update the visited rules during animation/transition.

during animation-only restyle.

::: servo/components/style/matching.rs:650
(Diff revision 1)
>              replacements,
>              context,
>              CascadeVisitedMode::Unvisited,
>              cascade_inputs,
>          );
> -        if !context.shared.traversal_flags.for_animation_only() {
> +        if cascade_inputs.primary.visited_rules.is_some() {

I think we can call replace_rules_internal() only if we needed just like you did in this patch, but this replace_rules() is called in normal traversal too, so I'd like to just drop the for_animation_only() check in this bug and will consider about the optimization in a later bug.
Attachment #8890256 - Flags: review?(hikezoe) → review+
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review166682

> I think we can call replace_rules_internal() only if we needed just like you did in this patch, but this replace_rules() is called in normal traversal too, so I'd like to just drop the for_animation_only() check in this bug and will consider about the optimization in a later bug.

Will do. Thanks for the review and making the commit message more accurate.
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review166844

::: commit-message-9eddb:4
(Diff revision 2)
> +Bug 1381235 - stylo: fix the optimization check in replace_rules() for visited styles.
> +
> +We skipped updating the rule nodes for visited rules during animation-only restyle.
> +However, this causes isseus that visited style overries animation styles on visited element.

Nit: overries -> overrides
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review166844

> Nit: overries -> overrides

Nice catch! Thank you. :)
Ok, it looks like I'm having a bit trouble making a decent test for this. The point is we would like to be able to test if a transition is run properly on a visited link.

At first, I want to do transitions on visited links, and try to get the computed styles to do the verification. However, getting computed styles from visited links is prohibited due to security concerns.

Then, I tried to write a reftest, but this is not a easy thing since the visited style is applied asynchronously.

So, I'd like to try if we could use the existing test framework for visited links [1], but haven't made a ready-for-review patch yet.

Hi hiro, I'll keep work on the test at the moment. At the meantime, do you think we should land this fix first?


[1] https://searchfox.org/mozilla-central/source/layout/style/test/test_visited_reftests.html
Flags: needinfo?(hikezoe)
Ok. fair enough.  let's land the fix first and leave this bug open and reuse this for test case.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> Ok. fair enough.  let's land the fix first and leave this bug open and reuse
> this for test case.

servo PR: https://github.com/servo/servo/pull/17889
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #21)
> At first, I want to do transitions on visited links, and try to get the
> computed styles to do the verification. However, getting computed styles
> from visited links is prohibited due to security concerns.

Not sure if it helps here, but there is `getVisitedDependentComputedStyle`[1] on DOMWindowUtils, which you can access via SpecialPowers, etc. as needed.

[1]: http://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1355
Ok, finally got this test ready for review.

I thought about make a simple reftest by using something like
```
<a href="">Visited Link</a>
```
, however, it seems like the visited style won't apply on the link element as expected. I guess it's because we run the test with clean history and profile, and we need to revisit the test to make the link really a visited one. Maybe that's the reason why we need test_visited_reftests.html to do the job.

I did verified that this test can pass in Gecko and Stylo (w/ the fix), but fail in Stylo (w/o the fix). Since reviewboard can't let me re-request review for this patch, I have to cancel the review from Bugzilla, and request again....sigh.
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

Re-request review for the test part.
Attachment #8890256 - Flags: review+ → review?(hikezoe)
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review168152

Initially I thought this test needs reftest-wait setup but after reading comments in test_visited_reftests.html. It's polling, interesting!

Thank you for writing this test!

::: layout/reftests/css-visited/transition-on-visited.html:12
(Diff revision 5)
> +<title>Test for Bug 1381235</title>
> +<style>
> +a {
> +  text-decoration: none;
> +  color: rgb(255, 0, 0);
> +  transition: all 1000s steps(2) -500s;

Nit: 'color' instead of 'all'.  Also if we use steps(2, start), the negative delay is not necessary.
Attachment #8890256 - Flags: review?(hikezoe) → review+
Comment on attachment 8890256 [details]
Bug 1381235 - add reftest for transitioning on visited links.

https://reviewboard.mozilla.org/r/161380/#review168152

> Nit: 'color' instead of 'all'.  Also if we use steps(2, start), the negative delay is not necessary.

Aha, good trick. Lesson learned!! Thanks for the review. :)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/140171ba59ec
add reftest for transitioning on visited links. r=hiro
https://hg.mozilla.org/mozilla-central/rev/140171ba59ec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.