|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
6.14 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
|Details | Review|
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...
2 months ago
2 months ago
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.
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!
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.
> 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.
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.
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)
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
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
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
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....
Updated the commit, thanks for help! If it's still r+, I will open a PR for Servo side changes. Is it?
Moved Servo side into: https://github.com/servo/servo/pull/16028
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f34476fa83fa Update test_selectors.html for better test coverage and update test expectations r=Manishearth
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.
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.
Gah, mid air collision undid the depends-on change.
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.
Comment on attachment 8849115 [details] Bug 1341642 - Add a reftest for :-moz-broken https://reviewboard.mozilla.org/r/121940/#review123944
Pushed by firstname.lastname@example.org: 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