Closed
Bug 1368113
Opened 7 years ago
Closed 7 years ago
Dynamic restyling of ::placeholder on <input type="number"> doesn't work correctly
Categories
(Core :: CSS Parsing and Computation, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: heycam)
References
Details
Attachments
(3 files)
Simple testcase:
<!DOCTYPE html>
<style>
input::placeholder { color: red }
input.green::placeholder { color: green; }
</style>
<input type="number" placeholder="This should be green">
<script>
var i = document.querySelector("input");
var s = getComputedStyle(i, "::placeholder");
// Make sure we've computed the old color.
var oldColor = s.color;
i.className = "green";
</script>
Presumably during the restyle we end up using the anonymous text input as the originating element instead of the number input...
This is not a problem with stylo, for what it's worth.
Changing this bit in GeckoRestyleManager.cpp:
if (frameElement->IsNativeAnonymous() &&
nsCSSPseudoElements::PseudoElementIsJSCreatedNAC(aPseudoType)) {
to drop the second part of that condition fixes things, as expected.
Cameron, I know there are comments there that say removing that exposes some XBL bugs, but are those more serious than the above?
Flags: needinfo?(cam)
|
Reporter | |
Comment 1•7 years ago
|
||
OK, the obvious fix does make layout/reftests/css-display/display-contents-xbl.xhtml fail, as expected.
The reason for that is that we have an underlying bug with XBL and display:contents, looks like. But in the test, this style:
.b::after {
content: 'c';
}
in the #b binding ends up working around that bug. The reason it works around it is that when ElementForStyleContext is called for the ::after frame, we have the following:
frameElement: The ::before element.
aParentContent: The <body>!
and then I presume we end up reframing things and end up with the right styles somehow on the reframe. The fix for this bug correctly makes us find the <span class="b"> as originatingElement for that ::after and return that, and hence not reframe.
If I remove that #b binding and just leave the #a test that ends up failing (the one with id "hostI"), then it fails with or without the fix for this bug. So all that's happening here is that this bug is causing incorrect behavior and in this one case leading to extra reframing, which is covering up a bug that we just have with XBL bits.
So I think we should fix this and either comment out that part of display-contents-xbl.xhtml or mark that test as failing or something.
|
Assignee | |
Comment 2•7 years ago
|
||
I think you're right that fixing the ::placeholder restyling is more important than fixing the XBL / display:contents restyling, for now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f9b9fb686bfab92b56549b327b3cab9e05028b0
Flags: needinfo?(cam)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Comment 6•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8876366 [details]
Bug 1368113 - Part 1: Test for restyling ::placeholder.
https://reviewboard.mozilla.org/r/147756/#review152154
Attachment #8876366 -
Flags: review?(bzbarsky) → review+
|
|
Reporter | |
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8876367 [details]
Bug 1368113 - Part 2: Split out problematic parts of display-contents-xbl.xhtml into separate tests.
https://reviewboard.mozilla.org/r/147758/#review152156
r=me, though I largely skimmed this
Attachment #8876367 -
Flags: review?(bzbarsky) → review+
|
|
Reporter | |
Comment 8•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8876368 [details]
Bug 1368113 - Part 3: Use closest non-NAC ancestor as originating element when restyling all NAC-implemented pseudo-elements.
https://reviewboard.mozilla.org/r/147760/#review152158
r=me. Thank you for doing this!
Attachment #8876368 -
Flags: review?(bzbarsky) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9817633686c2
Part 1: Test for restyling ::placeholder. r=bz
https://hg.mozilla.org/integration/autoland/rev/129c04db249e
Part 2: Split out problematic parts of display-contents-xbl.xhtml into separate tests. r=bz
https://hg.mozilla.org/integration/autoland/rev/68f9a27cdf3b
Part 3: Use closest non-NAC ancestor as originating element when restyling all NAC-implemented pseudo-elements. r=bz
|
|
||
Comment 10•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/9817633686c2
https://hg.mozilla.org/mozilla-central/rev/129c04db249e
https://hg.mozilla.org/mozilla-central/rev/68f9a27cdf3b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
||
Comment 11•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #1)
> OK, the obvious fix does make
> layout/reftests/css-display/display-contents-xbl.xhtml fail, as expected.
>
> The reason for that is that we have an underlying bug with XBL and
> display:contents, looks like. But in the test, this style:
>
> .b::after {
> content: 'c';
> }
>
> in the #b binding ends up working around that bug. The reason it works
> around it is that when ElementForStyleContext is called for the ::after
> frame, we have the following:
>
> frameElement: The ::before element.
> aParentContent: The <body>!
>
> and then I presume we end up reframing things and end up with the right
> styles somehow on the reframe. The fix for this bug correctly makes us find
> the <span class="b"> as originatingElement for that ::after and return that,
> and hence not reframe.
>
> If I remove that #b binding and just leave the #a test that ends up failing
> (the one with id "hostI"), then it fails with or without the fix for this
> bug. So all that's happening here is that this bug is causing incorrect
> behavior and in this one case leading to extra reframing, which is covering
> up a bug that we just have with XBL bits.
>
> So I think we should fix this and either comment out that part of
> display-contents-xbl.xhtml or mark that test as failing or something.
This is probably related to bug 1359656, btw.
See Also: → 1359656
|
||
Updated•7 years ago
|
Assignee: nobody → cam
You need to log in
before you can comment on or make changes to this bug.
Description
•