Closed Bug 1341642 Opened 6 years ago Closed 6 years ago

stylo: Need support for the various -moz stuff used for alt text


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




Tracking Status
firefox55 --- fixed


(Reporter: bzbarsky, Assigned: canova)


(Blocks 1 open bug)



(3 files, 2 obsolete files)


  data:text/html,<input type="image">

  data:text/html,<img alt="This should appear">

Neither one works in stylo.

This will need support for the following:

1)  The -moz-alt-content value for the "content" property.

2)  The :-moz-broken, :-moz-loading, :-moz-user-disabled, :-moz-suppressed, etc pseudo-classes.  While we're doing these we should do the :-moz-handler ones too, because we may have less test coverage for those...
Flags: needinfo?(simon.sapin) does the 'content: -moz-alt-content' part.

The pseudo classes are slightly more involved:

* In, NS_EVENT_STATE_* and NS_EVENT_STATE_HIGHEST_SERVO_BIT need to be updated so that relevant states have a bit value below NS_EVENT_STATE_HIGHEST_SERVO_BIT.

* The new states need to be added to (make sure the respective values match in both files)

* Then the pseudo-classes can be added to

I *think* that is all.
Flags: needinfo?(simon.sapin)
Nazim, I'm giving you a couple more glue for our 4-week sprint - let me know if you think you won't have time to get to them and I'll find another owner. Thanks for all your work on this stuff!
Assignee: nobody → canaltinova
Priority: -- → P1
First commit is complete but the second one is still WIP. There is a TODO to implement here but don't know exactly what to do here. Should I make a binding function that does the same thing as this part of the code?:

Simon, Could you look at this TODO please?
Boris, I couldn't find a documentation about parsing of `:-moz-empty-except-children-with-localname` pseudo-class. I think it just requires an identifier as argument. Could you check that please?
Also test cases are working correctly with these changes.

And note that there is another bug that changes the same parts of the code(bug 1341739). We need to wait for it first.
Flags: needinfo?(simon.sapin)
Flags: needinfo?(bzbarsky)
> I think it just requires an identifier as argument. Could you check that please?

That's correct.  It ends up going through CSSParserImpl::ParsePseudoClassWithIdentArg.  Of course you can do ":-moz-empty-except-children-with-localname(   something  )"; I assume the whitespace is dealt with by the tokenizer.
Flags: needinfo?(bzbarsky)
Yeah, tokenizer takes care of this. Thanks!
> Simon, Could you look at this TODO please?

Do you mean how to match :-moz-empty-except-children-with-localname(_) ? I have no idea, but yes the code you pointed to looks like it does something that corresponds to what the name suggests. You could either extract that C++ code to a function and add bindings for it, or port it to Rust based on existing bindings.
Flags: needinfo?(simon.sapin)
Not sure there's any reason to have an FFI call - it seems like we should reimplement this in pure rust.

We should probably implement a helper to do the ::empty bits, which we could use to fix bug 1343496 at the same time. We basically need to implement something like IsSignificantChild:

The length of a text node is just a field access, so we can get that using bindgen:
My work in bug 1341086 will handle :-moz-empty-except-children-with-localname(_)

(and all of the ident-arg non-ts pseudos)
Attachment #8847798 - Attachment is obsolete: true
Dropped the second commit since Manish is working on it and updated the tests expectations.
Comment on attachment 8847797 [details]
Bug 1341642 - Stylo: Add support for -moz-*  pseudo-classes for alt text
Attachment #8847797 - Flags: review?(manishearth) → review+
Comment on attachment 8848581 [details]
Bug 1341642 - Update test_selectors.html for better test coverage and update test expectations
Attachment #8848581 - Flags: review?(manishearth) → review+
Looks good. I guess these won't make many tests pass since the current set of enabled tests don't/can't touch alt text stuff)
We may not have any tests for this.  I just checked, and we do have some for Persian/Arabic alt text, but they use quirks mode and sized boxes so don't hit this codepath.

We should add a test if landing this doesn't cause any unexpected passes...
I added the test fix to the second commit. But the test stdout file became very large so I just pushed a try build for failing tests:
Flags: needinfo?(bzbarsky)
Thank you for reenabling this test coverage.  Found a bunch of bugs in both stylo and servo....
Flags: needinfo?(bzbarsky)
Updated the commit, thanks for help!

If it's still r+, I will open a PR for Servo side changes. Is it?
Pushed by
Update test_selectors.html for better test coverage and update test expectations  r=Manishearth
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
The landing of the servo part of this bug caused layout/reftests/bugs/315920-18b.html to fail. It's a :-moz-broken test, so it deserves investigation. I opened bug 1348689 about it.
No longer depends on: 1348689
Attachment #8848581 - Attachment is obsolete: true
Comment on attachment 8847797 [details]
Bug 1341642 - Stylo: Add support for -moz-*  pseudo-classes for alt text

I already changed the expectations in We should get bug 1348689 investigated. I suspect bug 1338982 may be the culprit, let's mark it as bloking it if so.
Attachment #8847797 - Flags: review?(emilio+bugs)
Gah, mid air collision undid the depends-on change.
Depends on: 1348689
Of course the failures can be explained because the Gecko side of this patch never landed. I've asked canaltinova to do a try run and update test expectations before landing it.
Note that the Gecko side of this patch enables tons of test coverage that caught the various bugs I filed on Friday night (like bug 1348487).  It would be good to get it landed ASAP, and whoever lands it will likely need to run test_selectors.html and do some last-minute expectation adjustments, if people are landing other fixes in parallel.  I'm happy to just pick up landing this tomorrow (Monday) morning that if people want.  Just let me know, please!
Interesting, I'm able to confirm that :-moz-broken is working fine with this patch and the reftest is passing on my local machine. But I couldn't see a passing test yesterday in try build. Triggered one more time to see if there is a change. But feel free to pick this up if there is something that I missed.
Ah, comment 35 is confused.  The parts it's talking about already landed in

So what's not landed is the nsEventStates changes, right?  That's slightly less urgent.

Reopening, since we do want to make sure we remember this.
Resolution: FIXED → ---
Comment on attachment 8849115 [details]
Bug 1341642 - Add a reftest for :-moz-broken
Attachment #8849115 - Flags: review?(emilio+bugs) → review+
Pushed by
Stylo: Add support for -moz-*  pseudo-classes for alt text r=manishearth
Add a reftest for :-moz-broken r=emilio
You need to log in before you can comment on or make changes to this bug.