Closed Bug 946184 Opened 11 years ago Closed 11 years ago

Sometimes style resolution goes wrong for the anonymous content of <input type=number>

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: ntim, Assigned: jwatt)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Screenshot
527.88 KB, image/png
Details
reduced testcase
752 bytes, text/html
Details
patch
15.75 KB, patch
bzbarsky
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached image Screenshot —
I'm not sure in which specific case the bug happens since it doesn't happen on some cases. The style isn't causing this I guess, or not fully since I tried adding background:red to the input, and the bug didn't happen. STR : - Flip on the pref dom.forms.number - Inspect element on an number input Try the number input on http://smartsearch.altervista.org/options.html for reference (the instant search delay is a good example) Result : - The "inner input" reveals Additional info : - Platform : Windows 8.1 - Focusing the input also does not work btw
Blocks: 344616
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Flags: needinfo?(jwatt)
This may go away with changes I'm making.
Flags: needinfo?(jwatt)
Depends on: 935508
Did bug 935508 (only _just_ landed on mozilla-inbound) help here?
(In reply to Jonathan Watt [:jwatt] from comment #2) > Did bug 935508 (only _just_ landed on mozilla-inbound) help here? It now has native styling but it still happens (at least on Windows). PDF.JS input is also affected (even before the patch landed).
Flags: needinfo?(jwatt)
Summary: input type="number" reveals inner input when inspecting element in some cases → Inspecting element on input[type=number] resets ::-moz-number-text styling
Summary: Inspecting element on input[type=number] resets ::-moz-number-text styling → Inspecting element on input[type=number] resets ::-moz-number-text styling (even in forms.css)
Summary: Inspecting element on input[type=number] resets ::-moz-number-text styling (even in forms.css) → Inspecting element on input[type=number] resets ::-moz-number-text styling in some cases (even in forms.css)
OK clarified the issue in the title, but it only happens in some cases, not all the time.
Okay, I can reproduce for HTML. Looking into it. How did you check pdf.js? (It'll the the same issue, but I'd like to take a look.)
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #5) > Okay, I can reproduce for HTML. Looking into it. > > How did you check pdf.js? (It'll the the same issue, but I'd like to take a > look.) Check any PDF file (in Firefox) with the adobe reader plugin disabled. PDF.JS is made in html too in case you didn't know.
(In reply to ntim007 from comment #6) > Check any PDF file I don't know of any pdfs with number spinners in them.
Oh, I see! The page number field is actually an <input type=number>. Hmm, I'm not sure that's desirable.
A minimal testcase would be useful here.
So far I've reduced this to https://jwatt.org/tmp/946184/options.html and it reproduces on all platforms. It seems to me that the |input[type=number]::-moz-number-text| pseudo-element styling rule in forms.css is not applying in some cases. This seems to be some sort of race. In the original altravista page it fails on initial load and hard refresh, and works on a soft refresh. In the reduced testcase it reproduces all the time. The rule I'm talking about is this: http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=b789f8ff7590&mark=904-904#903 Boris, any tips or debugging this?
Flags: needinfo?(bzbarsky)
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Inspecting element on input[type=number] resets ::-moz-number-text styling in some cases (even in forms.css) → input[type=number]::-moz-number-text pseudo-element styling in forms.css not applying in some cases (race)
Yeah, this hack fixed is, so we have a workaround for v28 if we need it: diff --git a/layout/style/forms.css b/layout/style/forms.css @@ -894,16 +894,17 @@ input[type="number"] { float: none !important; position: static !important; } +input[type="number"] > div > input, input[type=number]::-moz-number-text { -moz-appearance: none; /* work around autofocus bug 939248 on initial load */ -moz-user-modify: read-write;
The most likely cause would be some sort of dynamic restyling failure where we fail to give that element a style context with the pseudo-element attached, right? In fact, if I just load this document with devtools inspector open I get the failure: <!DOCTYPE html> <body onload="document.body.style.backgroundColor = 'yellow'"> <input type="number" style="width:50%"> </body>
Flags: needinfo?(bzbarsky)
What I don't know is why devtools matter here.
It also resets styling outside forms.css. Just to clarify since you changed the title. Also, I noticed that applying moz appearance none to the spin box buttons makes them unusable.
(In reply to ntim007 from comment #14) > It also resets styling outside forms.css. Just to clarify since you changed > the title. I'm not sure what you mean. I'm seeing the red border applied even when the pseudo-element styling fails on https://jwatt.org/tmp/946184/options.html Can you be much more specific about concrete things you're seeing in specific pages?
Sorry. I meant the ::-moz-number-text pseudo element styling. ALL styling applied on it will be reset in those specific cases you discovered even the styling outside forms.css
Just to be clear other number input related styling are not affected by this bug.
(In reply to ntim007 from comment #16) > Sorry. I meant the ::-moz-number-text pseudo element styling. ALL styling > applied on it will be reset in those specific cases you discovered Yes, if the rule doesn't apply, all the styling specified by that rule will not apply. > even the styling outside forms.css This is the bit I don't understand. There is no ::-moz-number-text styling outside forms.css. Are you saying you've tried putting ::-moz-number-text rules in content style sheets and they haven't worked? If so, that's expected, since content is not allowed to access ::-moz-number-text at all.
(In reply to Jonathan Watt [:jwatt] from comment #18) > (In reply to ntim007 from comment #16) > > Sorry. I meant the ::-moz-number-text pseudo element styling. ALL styling > > applied on it will be reset in those specific cases you discovered > > Yes, if the rule doesn't apply, all the styling specified by that rule will > not apply. > > > even the styling outside forms.css > > This is the bit I don't understand. There is no ::-moz-number-text styling > outside forms.css. Are you saying you've tried putting ::-moz-number-text > rules in content style sheets and they haven't worked? If so, that's > expected, since content is not allowed to access ::-moz-number-text at all. I can still style the pseudo elements using a stylish userstyle, and that's why I said even outside forms.css. And it's weird that content isn't allowed to access those pseudo elements.
Ah, stylish installs its userstyles as UA sheets. So that part makes sense. > And it's weird that content isn't allowed to access those pseudo elements. Not at all. They're nonstandard, and we don't want to be dumping random nonstandard stuff on the web.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Boris Zbarsky [:bz] from comment #20) > Ah, stylish installs its userstyles as UA sheets. So that part makes sense. > > > And it's weird that content isn't allowed to access those pseudo elements. > > Not at all. They're nonstandard, and we don't want to be dumping random > nonstandard stuff on the web. You've got a point, but it's still nice to be able to freely style the controls like the range input.
Sure, and we should come up with a spec for it, then expose it.
Further reduced this to https://jwatt.org/tmp/946184/reduced.html but this time the thing that's going wrong with the anonymous contents' style contexts is that they are not inheriting style from the <input type=number>. The workaround introduced in bug 947718 doesn't help, of course.
Ah, and here is a very reduced testcase with no external dependencies: https://jwatt.org/tmp/946184/reduced2.html
Attached file reduced testcase (obsolete) —
Attached file reduced testcase —
Attachment #8344714 - Attachment is obsolete: true
What are they inheriting from?
(In reply to Boris Zbarsky [:bz] from comment #27) > What are they inheriting from? At least in some cases the anon text control's style context's rule node's parent is null. I think things are going wrong under this stack when GetContext is called for the anon text control: nsStyleSet::GetContext nsStyleSet::ResolveStyleFor nsCSSFrameConstructor::ResolveStyleContext nsCSSFrameConstructor::ResolveStyleContext nsCSSFrameConstructor::AddFrameConstructionItems nsCSSFrameConstructor::ContentRangeInserted nsCSSFrameConstructor::ContentInserted nsCSSFrameConstructor::RecreateFramesForContent mozilla::RestyleManager::ProcessRestyledFrames mozilla::RestyleManager::DoRebuildAllStyleData mozilla::RestyleManager::RebuildAllStyleData mozilla::RestyleManager::ProcessPendingRestyles PresShell::FlushPendingNotifications nsRefreshDriver::Tick aPseudoType is ePseudo_NotPseudoElement, because that's what nsStyleSet::ResolveStyleFor passes in. It seems that ProcessRestyledFrames is calling RecreateFramesForContent on the anon text control only (not the number control or any of the other anon elements). If it does that then I don't see mow the new style context is supposed to get the correct pseudo-element associated with it. I see code to do that when a restyle occurs (not sure if it always happens even in that case), but I don't see anything for when there's a reframe. Even if we have some mechanism somewhere to make this happen, it's failing and we end up with a borked nsStyleContext. (If we always made sure that recreating frames for the number control's anonymous content recreated the frame for the number control itself then I think maybe we wouldn't have a problem. I'm not sure how all this would work if things were going as intended though.) Regarding how we come to be processing nsChangeHint_ReconstructFrame for the anon text field, at some earlier point we end up calling nsStyleDisplay::CalcDifference for the anon text control, and that returns that reconstruct hint because mDisplay has changed between 'inline' and 'block'. It seems likely that the inline/block change is something to do with the fact that we're using flexbox layout to lay out the anon content (nsStyleContext::ApplyStyleFixups changes mDisplay). Exactly why it is that sometimes we get inline and sometimes block, I don't know yet.
Adding: input[type=number] > div > input, /* work around bug 946184 */ input[type=number]::-moz-number-text { display: block; } to forms.css "fixes" all the pages/testcases discussed in this bug, so dholbert thinks at least part of this is a form of bug 926670. I'm still concerned that there may be other ways that we can end up reconstructing the frames for the anon text control without reconstructing the frames for the number control though, and end up with a broken style context for the text control some other way.
nsCSSFrameConstructor::RecreateFramesForContent is supposed to go up to the nearest thing that it's safe to recreate. It looks like right now it only does this for generated content, but it seems like it should also do this for any pseudo-element frame at the very least. Or possibly for anything that's native anonymous content (note that for _root_ native anonymous content it already does this as well).
Setting a frame bit on frames to indicate that they are nsIAnonymousContentCreator content is not straightforward. The frames are created from FrameContstructionItem objects created by nsCSSFrameConstructor::AddFrameConstructionItemsInternal under the call: nsCSSFrameConstructor::AddFrameConstructionItemsInternal nsCSSFrameConstructor::AddFCItemsForAnonymousContent nsCSSFrameConstructor::ProcessChildren after which they're processed when nsCSSFrameConstructor::ProcessChildren calls ConstructFramesFromItemList. By that time the itemsToConstruct object passed to that call contains a mix of non-anonymous items though. And as you pointed out in bug 917386 the FrameContstructionItem objects are static const data, so I can't get AddFrameConstructionItemsInternal to add a flag to them when under the AddFCItemsForAnonymousContent call.
FrameConstructionData objects are static const. FrameConstructionItem objects are not static and not const, and store all sorts of flags that you can set as needed.
Attached patch patch (obsolete) — — Splinter Review
Couple of XXXjwatt comments in there that I'm not sure about. Seems like these points could create intermediary frames between the nsIAnonymousContentCreator and some of the frames created for the anon content that would not have the necessary state bit set.
Assignee: nobody → jwatt
Attachment #8345261 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #32) > FrameConstructionData objects are static const. FrameConstructionItem > objects are not static and not const, and store all sorts of flags that you > can set as needed. Err, right. Working until 5:30am after a few weeks of brain frazzling overworking trying to get a project out the door seems to make obvious things less obvious. ;)
Blocks: 948433
Comment on attachment 8345261 [details] [diff] [review] patch >+ // XXX should this be above the item.mIsText check? Probably doesn't matter, since we never try to reconstruct text frames directly. However changing aState.mAdditionalStateBits seems wrong: that will propagate the flag to all descendant frames, and in particular will mess up the videocontrols case we didn't want to mess up, no? For the generated content case we explicitly _want_ that behavior, but for our case I don't think we do. In practice, we only need the flag on items that were explicitly based on stuff that came back from CreateAnonymousContent, so I think we should just have ConstructFrameFromItemInternal look at the mIsAnonymousContentCreatorContent boolean and set the frame flag as needed. >+ "the way no the native anonymous root - we're going to " "the way _to_ the native anonymous root..", no? >+ while (anonRoot->GetParent()->GetStateBits() & >+ NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT) { No, this doesn't seem right, precisely because of the issue you raised with random wrapper frames... We could do something with skipping anon boxes here, or we could just walk up to the nearest nsIAnonymousContentCreator. I think I prefer the former, because the latter would get all confused if a <select> ended up in native anon content (yeah, I know, who would do that?).
Attachment #8345261 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #35) > However changing aState.mAdditionalStateBits seems wrong: that will > propagate the flag to all descendant frames, and in particular will mess up > the videocontrols case we didn't want to mess up, no? For the generated > content case we explicitly _want_ that behavior, but for our case I don't > think we do. Oh, I see, the videocontrols.xml#videoControls binding is bound to an element created by nsVideoFrame::CreateAnonymousContent. Then yes, that would be bad! > >+ while (anonRoot->GetParent()->GetStateBits() & > >+ NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT) { > > No, this doesn't seem right, precisely because of the issue you raised with > random wrapper frames... > > We could do something with skipping anon boxes here, or we could just walk > up to the nearest nsIAnonymousContentCreator. I don't think we can assume that leaving intermediary frames without the state bit set is safe. What if one of the intermediary frames is the frame that is being recreated, and a child of that frame is a frame that's been created from a ContentInfo that sets an nsStyleContext with mPseudoTag set? I think as we construct the frames we need to make sure the bit is set up to the nsIAnonymousContentCreator.
Attached patch patch — — Splinter Review
How about this?
Attachment #8345261 - Attachment is obsolete: true
Attachment #8345645 - Flags: review?(bzbarsky)
Attachment #8345645 - Attachment is patch: true
> What if one of the intermediary frames is the frame that is being recreated "frames" are never recreated, luckily. nsIContent is the argument to RecreateFramesForContent. These various wrapper frames (table pseudos, the XUL box wrapper, etc) don't have their own content nodes; they get the content of their parent frame.
Comment on attachment 8345645 [details] [diff] [review] patch So my main annoyance here is that this optimizes the loop in RecreateFramesForContent (which imo should be rare for the NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT case) at the expense of the more common creation of the frames. So even if we just want to walk up to the nsIAnonymousContentCreator in RecreateFramesForContent I would much prefer that. As I noted, we should never have a reframe coming through there which would reframe some NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT descendants but not have that bit itself. >nsCSSFrameConstructor::ConstructFrameFromItemInternal(FrameConstructionItem& aItem, There is no need to put the code in this code twice. We should simply set the flag on primaryFrame at the end of the method, since that's the frame we'll end up working with in RecreateFramesForContent, right? >+ while (anonRoot->GetParent()->GetStateBits() & >+ NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT) { Weird indent there. r=me modulo the performance concern and the nits.
Attachment #8345645 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #39) > As I noted, we should never have a reframe coming through there which would > reframe some NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT descendants but not > have that bit itself. I thought that XBL bindings with <children> on one of the NA elements could make that happen, but you would definitely know better about that than me.
Summary: input[type=number]::-moz-number-text pseudo-element styling in forms.css not applying in some cases (race) → Sometimes style resolution goes wrong for the anonymous content of <input type=number>
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d6f276f58d I added a special case for SVG <use>. Please take one last look at the commit. Thanks, Boris! And thanks, Tim, for your early testing and bug reporting!
> I thought that XBL bindings with <children> on one of the NA elements could make that > happen Hmm. It could, in theory, but that setup is not compatible with the ContentInfo bits in any way as far as I can tell (because the XBL stuff doesn't know about ContentInfo and vice versa). So let's not worry about it. ;) > I added a special case for SVG <use>. Hmm. Why is that one OK? What makes <use> not need the special reframing behavior?
https://hg.mozilla.org/mozilla-central/rev/73d6f276f58d
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Boris Zbarsky [:bz] from comment #42) > Hmm. Why is that one OK? What makes <use> not need the special reframing > behavior? Because nsSVGUseFrame::CreateAnonymousContent simply creates a clone of the content of an existing document fragment, without ever providing a custom style context.
Comment on attachment 8345645 [details] [diff] [review] patch Bug caused by (feature/regressing bug #): 344616 and its dependencies User impact if declined: bad number field breakage, such as the page number field in pdf.js being badly broken Testing completed (on m-c, etc.): landed m-i, merged to m-c Risk to taking this patch (and alternatives if risky): low, and early in cycle String or IDL/UUID changes made by this patch: none We're early in the cycle so hopefully this is fine to uplift. Probably needed to keep <input type=number> on in v28.
Attachment #8345645 - Flags: approval-mozilla-aurora?
> without ever providing a custom style context. Hmm. I guess we can't just set the flag on things with custom styles because they might have custom-style descendants... Can we restrict setting the flag on things that come from ContentInfo and either have a custom style or ContentInfo descendants and get rid of the <use> hack? Followup for this is ok.
I think so. I began a patch to use a RAII class to avoid setting the frame state bit unless there's something further up the stack that provided a style context. It'll be January before I look at that properly though.
Filed bug 949383.
Depends on: 949383
Attachment #8345645 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox28: --- → fixed
Target Milestone: mozilla29 → mozilla28
The tarket milestone tracks when the bug landed on m-c.
status-firefox29: --- → fixed
Target Milestone: mozilla28 → mozilla29
Verified fixed on nightly 20131212030202 build.
status-firefox29: fixed → verified
Verified fixed on aurora 28.0a2 20131213004002 build.
Status: RESOLVED → VERIFIED
status-firefox28: fixed → verified
QA Contact: ananuti
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: