Closed Bug 1370358 Opened 6 years ago Closed 6 years ago

Stylo: Fix issues with nested link matching

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1370358 - Log element during selector matching.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1370358 - Stop looking for relevant links once found.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1370358 - Enable visited selector tests.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.