Stylo: Fix issues with nested link matching

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

Stylo's handling of visitedness doesn't quite match Gecko's in the case of links inside of links.

In particular, this affects tests like:

layout/reftests/css-visited/selector-child-2.xhtml
layout/reftests/css-visited/selector-descendant-2.xhtml
Comment on attachment 8875035 [details]
Bug 1370358 - Log element during selector matching.

https://reviewboard.mozilla.org/r/146392/#review150540

::: servo/components/style/restyle_hints.rs:615
(Diff revision 1)
> +impl<'a, E> fmt::Debug for ElementWrapper<'a, E>
> +    where E: TElement,
> +{
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        // Ignore other fields for now, can change later if needed.
> +        self.element.fmt(f)

I'd just log `self.snapshot()` too, but no big deal.
Attachment #8875035 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875036 [details]
Bug 1370358 - Stop looking for relevant links once found.

https://reviewboard.mozilla.org/r/146394/#review150542

::: servo/components/selectors/matching.rs:227
(Diff revision 1)
>                                   -> RelevantLinkStatus
>          where E: Element,
>      {
> -        if *self != RelevantLinkStatus::Looking {
> +        // If a relevant link was previously found, we no longer want to look
> +        // for links.  Only the nearest ancestor link is considered relevant.
> +        if *self == RelevantLinkStatus::Found {

Why not:

```
if *self != RelevantLinkStatus::Looking {
    return RelevantLinkStatus::NotLooking;
}
```

(Keeping the same comment)
Attachment #8875036 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875037 [details]
Bug 1370358 - Enable visited selector tests.

https://reviewboard.mozilla.org/r/146396/#review150544
Attachment #8875037 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875035 [details]
Bug 1370358 - Log element during selector matching.

https://reviewboard.mozilla.org/r/146392/#review150540

> I'd just log `self.snapshot()` too, but no big deal.

For my current usage, it was nicer to leave it as just the element to reduce the noise and to have the same format for elements and wrappers.  But, sure someone else may have a different need.  So, I think I'll leave it as-is for the moment.
Comment on attachment 8875036 [details]
Bug 1370358 - Stop looking for relevant links once found.

https://reviewboard.mozilla.org/r/146394/#review150542

> Why not:
> 
> ```
> if *self != RelevantLinkStatus::Looking {
>     return RelevantLinkStatus::NotLooking;
> }
> ```
> 
> (Keeping the same comment)

Okay, makes sense, I've changed it. :)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b129d01e825c
Enable visited selector tests. r=emilio
https://hg.mozilla.org/mozilla-central/rev/b129d01e825c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.