Closed Bug 1379696 Opened 7 years ago Closed 7 years ago

stylo: -moz-any(...):before applies too broadly (causing mismatched backgrounds behind icons at Evite)

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: dholbert, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

testcase 1
365 bytes, text/html
Details
screenshot of bug on evite
21.80 KB, image/png
Details
screenshot of expected rendering (taken with stylo pref turned off)
19.76 KB, image/png
Details
testcase 2
350 bytes, text/html
Details
Bug 1379696: Reftest.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Attached file testcase 1 — 
STR:
 0. Set pref layout.css.servo.enabled to true, and restart Firefox.
 1. Load attached testcase.

EXPECTED RESULTS:
 There should *not* be an orange background behind first line of text.

ACTUAL RESULTS:
 There's an orange background behind all text.


It looks like :-moz-any(...) is being treated as matching every element, or something like that.
Blocks: stylo-site-issues
I ran into this bug on Evite. I created a test evite page that demonstrates the bug.

To see it there, first you need to RSVP to my test "event" here:
  https://www.evite.com/event/02072XBCHGW7NI57YEPHMWHB7G322Q/rsvp
and then view this "activity" page:
 https://www.evite.com/event/02072XBCHGW7NI57YEPHMWHB7G322Q/activity

If you've RSVP'd, you'll see "heart" icons that let you react to the comments.  For the comments that are shown with a gray background (which are responses-to-comments), the heart is shown with an odd white background.  This is due to a misapplied -moz-any CSS rule.
Summary: With stylo enabled, -moz-any(...):before applies too broadly (causing mismatched backgrounds behind icons at Evite) → stylo: -moz-any(...):before applies too broadly (causing mismatched backgrounds behind icons at Evite)
This bug also causes a weird "veil" over my avatar shown on the sample evite page, too. (And to see that, you don't have to even RSVP -- you can just go straight to the evite "activity" page and look at the little cartoon avatar.  If you zoom in, you can see there's a weird transparent-white top-of-a-circle going across the face part.)

    
  

        Attachment #8884899 -
        Attachment description: screenshot of expected rendering → screenshot of expected rendering (taken with stylo pref turned off)

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 2
        — 
      

    


  
This testcase exercises -moz-any(.foo) as well as -moz-any(.foo):before, and it looks like only the latter selector is broken.

Regardless of the stylo pref, the -moz-any(.foo) rule seems to apply correctly (only to the "FOO") div.  But the -moz-any(.foo):before rule seems to apply to every rendered element (including even <html> and <body>) when Stylo is enabled.

    
    

    
  
I'll see if I can resolve this.
Assignee: nobody → bwerth

    
    

    
  
This trips quite a few assertions in debug builds, and it's easy to fix. I just wrote a quick patch for it, though I don't know if Brad had something else in mind.

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
Err, I didn't meant to publish them until talking with Brad, but did mozreview push in the wrong tree, dammit. Sorry, I'll cancel review for now. That's my proposed solution, but feel free to steal or whatever if you arrive to other more elegant solution (which I believe is possible).

    
  

        Attachment #8884981 -
        Flags: review?(bobbyholley)

    
  

        Attachment #8884982 -
        Flags: review?(bobbyholley)

    
    

    
  
(Brad said he was ok with me taking this. Sorry again for publishing review requests too fast :/)

    
  

        Attachment #8884981 -
        Flags: review?(bobbyholley)

    
  

        Attachment #8884982 -
        Flags: review?(bobbyholley)
Assignee: bwerth → emilio+bugs

    
    

    
  
Comment on attachment 8884981 [details]
Bug 1379696: Track the selector recursion level, and avoid looking at MatchingMode if the selector isn't the topmost.

https://reviewboard.mozilla.org/r/155806/#review161026

r=me with those two things fixed.

::: servo/components/selectors/context.rs:101
(Diff revision 1)
> +    /// The amount of selectors we've been recursing into.
> +    pub depth: usize,

I'd do:

/// The level of nesting for the selector being matched.
pub nesting_level: usize,

Additionally, I think this makes more sense in LocalMatchingContext rather than shared, since it's very much a property of the current selector being matched. So please move it there.

        Attachment #8884981 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
Comment on attachment 8884982 [details]
Bug 1379696: Reftest.

https://reviewboard.mozilla.org/r/155808/#review161028

        Attachment #8884982 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
Comment on attachment 8884981 [details]
Bug 1379696: Track the selector recursion level, and avoid looking at MatchingMode if the selector isn't the topmost.

https://reviewboard.mozilla.org/r/155806/#review161026

> I'd do:
> 
> /// The level of nesting for the selector being matched.
> pub nesting_level: usize,
> 
> Additionally, I think this makes more sense in LocalMatchingContext rather than shared, since it's very much a property of the current selector being matched. So please move it there.

Sounds good, will do.

    
    

    
  
Comment on attachment 8884981 [details]
Bug 1379696: Track the selector recursion level, and avoid looking at MatchingMode if the selector isn't the topmost.

https://reviewboard.mozilla.org/r/155806/#review161026

> Sounds good, will do.

I'll fix the docs of LocalMatchingContext, btw, which are confusing (they don't store per-element data).

    
    

    
      
  

    
  

        Attachment #8884981 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/24674ef44c14
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox56:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

    
    

    
  
Nightly 56 x64 20170711100226 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)
testcase 1 (attachment 8884884 [details]) is not fixed in this build.
Maybe tomorrow? Will retest.

    
    

    
  
Verified fixed in Nightly 56 x64 20170712100301 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480):
testcase 1 + 2 now look the same with and without stylo.
Status: RESOLVED → VERIFIED
Has STR: --- → yes

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: