1383307 - stylo: site issue: visited links on BMO are not purple if you collapse and expand "Tracking"

Status: VERIFIED FIXED

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

Description

[Darkspirit]

Attachment: 2017-07-22_00-15-21.mp4

Nightly 56 x64 20170721100159 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480)

I can't reproduce bug 1380543 anymore, but this one is similar.

STR:
1. open https://bugzilla.mozilla.org/show_bug.cgi?id=1375906
2. collapse and expand "Tracking"
3. move your mouse over all blue (visited) "Depends on" links, they get purple
4. repeat (2), all links are blue again

See video.

Comment 1

[Darkspirit]

If I zoom in/out or resize the window all blue (visited) links get purple (as they should have been).

Comment 2

[Jeremy Chen ([e-mail])]

FWIW, I can't reproduce this on today's Nightly on Mac OS X 10.12. Not sure if this is Linux specific...

Version: 	56.0a1
Build ID: 	20170721100207
OS:  	        Darwin 16.6.0

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> FWIW, I can't reproduce this on today's Nightly on Mac OS X 10.12. Not sure
> if this is Linux specific...
> 
> Version: 	56.0a1
> Build ID: 	20170721100207
> OS:  	        Darwin 16.6.0

I have seen this several times over the last few days and I'm on Windows. I'll attach a screenshot of what I have seen. Unfortunately this issue isn't one I have been able to physically reproduce, it seems to happen sporadically and thus I have been unable to build a testcase for it.

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: visited links not marked as visited.PNG

This shows the display view from Mozilla inbound. As you can see some of the links for bug 1380133 are styled as visited and some are not.

As they are all the same link I would have expected them all to be styled the same.

Moving the cursor over the links leads to the styles updating.

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

:emilio - Have you seen any bugs like this crop up in Stylo by any chance?

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

Actually it seems that the issue is fully reproducible on todays nightly with the steps as outlined in comment #0.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yes, this is fully reproducible...

I wasn't aware of this, but after taking a look I know what this is, and this is fallout for the huge rubberstamp that was bug 1367904, and yet another change that should've been done before that bug... :(

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1383307: Use proper initializer in ServoStyleContext constructor.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/160156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160156/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Fix relevant-link-visited logic in presence of nested links.

And kill one GetParentAllowServo call while at it, and some other dumbness...

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/160158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160158/

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889123 [details]
Bug 1383307: Use proper initializer in ServoStyleContext constructor.

https://reviewboard.mozilla.org/r/160156/#review165494

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889124 [details]
style: Fix relevant-link-visited logic in presence of nested links.

https://reviewboard.mozilla.org/r/160158/#review165496

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1383307: Honor the relevant link visited pref.

Review commit: https://reviewboard.mozilla.org/r/160260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160260/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: stylo: Set the relevant link visited bit from servo.

Review commit: https://reviewboard.mozilla.org/r/160262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160262/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1383307: Remove ServoStyleContext::UpdateWithElementState.

And kill one GetParentAllowServo call while at it, and some other dumbness...

Review commit: https://reviewboard.mozilla.org/r/160264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160264/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8889123 [details]
Bug 1383307: Use proper initializer in ServoStyleContext constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160156/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8889124 [details]
style: Fix relevant-link-visited logic in presence of nested links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160158/diff/1-2/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

The second patch ("fix visited logic...") is what needs review, not the last one.

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889123 [details]
Bug 1383307: Use proper initializer in ServoStyleContext constructor.

https://reviewboard.mozilla.org/r/160156/#review165520

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889124 [details]
style: Fix relevant-link-visited logic in presence of nested links.

https://reviewboard.mozilla.org/r/160158/#review165522

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889161 [details]
Bug 1383307: Honor the relevant link visited pref.

https://reviewboard.mozilla.org/r/160260/#review165524

::: servo/components/style/context.rs:121
(Diff revision 1)
>  pub struct SharedStyleContext<'a> {
>      /// The CSS selector stylist.
>      pub stylist: &'a Stylist,
>  
> +    /// Whether visited styles are enabled.
> +    pub visited_styles_enabled: bool,

nit: leave a comment on when they won't be enabled

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889162 [details]
stylo: Set the relevant link visited bit from servo.

https://reviewboard.mozilla.org/r/160262/#review165526

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8889163 [details]
Bug 1383307: Remove ServoStyleContext::UpdateWithElementState.

https://reviewboard.mozilla.org/r/160264/#review165528

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8889123 [details]
Bug 1383307: Use proper initializer in ServoStyleContext constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160156/diff/2-3/

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8889161 [details]
Bug 1383307: Honor the relevant link visited pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160260/diff/1-2/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8889163 [details]
Bug 1383307: Remove ServoStyleContext::UpdateWithElementState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160264/diff/1-2/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e170c7eca2c4
Use proper initializer in ServoStyleContext constructor. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/c92ef054df2b
Honor the relevant link visited pref. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/469e3f8d6359
Remove ServoStyleContext::UpdateWithElementState. r=manishearth

Comment 27

[Emilio Cobos Álvarez (:emilio)]

BTW, I tried to build a test-case for this, but I found hard to write something that reliably failed before my patches because of the async link coloring stuff, which kinda sucks :(

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8889123 [details]
Bug 1383307: Use proper initializer in ServoStyleContext constructor.

https://reviewboard.mozilla.org/r/160156/#review165582

(Marking r+ for posterity, since mozreview didn't seem to want to forget about my review request after Manish reviewed.)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e170c7eca2c4
https://hg.mozilla.org/mozilla-central/rev/c92ef054df2b
https://hg.mozilla.org/mozilla-central/rev/469e3f8d6359

Comment 30

[Darkspirit]

Verified fixed in Nightly 56 x64 20170725100346 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480).