Closed Bug 1341642 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

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

People

(Reporter: bzbarsky, Assigned: canova)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Bug 1341642 - Stylo: Add support for -moz-* pseudo-classes for alt text
59 bytes, text/x-review-board-request
Details
Additional updates to the test and expectation updates
6.14 KB, patch
Details | Diff | Splinter Review
Bug 1341642 - Add a reftest for :-moz-broken
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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: stylo-reftest
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
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)
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)
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

https://reviewboard.mozilla.org/r/120706/#review123528
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

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...
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)
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 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
Closed: 7 years ago
status-firefox55: --- → fixed
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

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
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 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 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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: