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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: bz, Assigned: canaltinova)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

Testcases:

  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)
Blocks: 1324348
https://github.com/servo/servo/pull/15691 does the 'content: -moz-alt-content' part.


The pseudo classes are slightly more involved:

* In, http://searchfox.org/mozilla-central/source/dom/events/EventStates.h 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 http://searchfox.org/mozilla-central/source/servo/components/style/element_state.rs (make sure the respective values match in both files)

* Then the pseudo-classes can be added to http://searchfox.org/mozilla-central/source/servo/components/style/gecko/non_ts_pseudo_class_list.rs

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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
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?: http://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1779-1802

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)
(Assignee)

Comment 7

3 months ago
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: http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/layout/style/nsStyleUtil.cpp#726

The length of a text node is just a field access, so we can get that using bindgen:
http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/dom/base/nsTextFragment.h#103
My work in bug 1341086 will handle :-moz-empty-except-children-with-localname(_)

(and all of the ident-arg non-ts pseudos)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8847798 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 13

3 months ago
Dropped the second commit since Manish is working on it and updated the tests expectations.

Comment 14

3 months ago
mozreview-review
Comment on attachment 8847797 [details]
Bug 1341642 - Stylo: Add support for -moz-*  pseudo-classes for alt text

https://reviewboard.mozilla.org/r/120706/#review123528
Attachment #8847797 - Flags: review?(manishearth) → review+

Comment 15

3 months ago
mozreview-review
Comment on attachment 8848581 [details]
Bug 1341642 - Update test_selectors.html for better test coverage and update test expectations

https://reviewboard.mozilla.org/r/121488/#review123530
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...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

3 months ago
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:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e1aa9e9b4fbe8eac3942edd2308dc8d9b749b60
Flags: needinfo?(bzbarsky)
Created attachment 8848709 [details] [diff] [review]
Additional updates to the test and expectation updates

Thank you for reenabling this test coverage.  Found a bunch of bugs in both stylo and servo....
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

3 months ago
Updated the commit, thanks for help!

If it's still r+, I will open a PR for Servo side changes. Is it?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

3 months ago
Moved Servo side into: https://github.com/servo/servo/pull/16028

Comment 27

3 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f34476fa83fa
Update test_selectors.html for better test coverage and update test expectations  r=Manishearth
https://hg.mozilla.org/mozilla-central/rev/f34476fa83fa
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348689
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
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8848581 - Attachment is obsolete: true

Comment 31

3 months ago
mozreview-review
Comment on attachment 8847797 [details]
Bug 1341642 - Stylo: Add support for -moz-*  pseudo-classes for alt text

https://reviewboard.mozilla.org/r/120706/#review123740

I already changed the expectations in https://hg.mozilla.org/integration/autoland/rev/c6e919aa3aa9. 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
Comment hidden (mozreview-request)
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!
Comment hidden (mozreview-request)
(Assignee)

Comment 37

3 months ago
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 https://hg.mozilla.org/integration/autoland/rev/f34476fa83fa

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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

3 months ago
mozreview-review
Comment on attachment 8849115 [details]
Bug 1341642 - Add a reftest for :-moz-broken

https://reviewboard.mozilla.org/r/121940/#review123944
Attachment #8849115 - Flags: review?(emilio+bugs) → review+

Comment 42

3 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0739f669ec25
Stylo: Add support for -moz-*  pseudo-classes for alt text r=manishearth
https://hg.mozilla.org/integration/autoland/rev/c7d08a2d8d49
Add a reftest for :-moz-broken r=emilio
(Assignee)

Updated

3 months ago
Depends on: 1348873

Comment 43

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0739f669ec25
https://hg.mozilla.org/mozilla-central/rev/c7d08a2d8d49
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.