Closed Bug 534458 Opened 16 years ago Closed 15 years ago

[HTML5] test failure on css3.info selectors test with :checked{}

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: phiw2, Unassigned)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

Reftest
1.28 KB, patch
Details | Diff | Splinter Review
Testcase showing problem with old parser
384 bytes, text/html
Details
The last test (partly) fails with html5 enabled: :checked { } <input type='checkbox' checked> http://www.css3.info/selectors-test/test-checked.html#checked
Whether an element matches :checked is coming from the style system calling nsHTMLInputElement::IntrinsicState. So I would presume the bug is either in parser or content code. The question is why that function doesn't add the NS_EVENT_STATE_CHECKED bit.
That said, this works fine for me. What build are you using?
(In reply to comment #2) > That said, this works fine for me. What build are you using? Gecko/20091212 Minefield/3.7a1pre OS X rev/d379a17cbf8f On http://www.css3.info/selectors-test/test-checked.html#checked I get a red background (On Linux as well) It is a very recent regression (between 12-11 and 12-12), I'm looking to narrow it down further.
Component: Style System (CSS) → HTML: Parser
QA Contact: style-system → parser
Probably yes. From available tinderbox builds (OS X) I get the change: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=25ac71dc0b39&tochange=9e0aa4f1927b
Component: HTML: Parser → Style System (CSS)
QA Contact: parser → style-system
Component: Style System (CSS) → HTML: Parser
QA Contact: style-system → parser
I see this too with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091212 Minefield/3.7a1pre
I see this too, until I open Firebug or hover over the checkbox (?!). Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091212 Minefield/3.7a1pre
(In reply to comment #7) > I see this too, until I open Firebug or hover over the checkbox (?!). Hmmm. That makes it sound like the parser is setting the attribute but not sending appropriate notifications for it.
Bug 497861 was supposed to make form widget state tracking better. Not good enough, it seems.
Priority: -- → P1
Hmm. Are we hitting reparse here? Or state preservation issues? Or is the parser setting that attribute after inserting the node in the DOM? Which order are the type and checked attributes set in, in this case?
Blocks: html5-parsing
(In reply to comment #10) > Hmm. Are we hitting reparse here? Or state preservation issues? Or is the > parser setting that attribute after inserting the node in the DOM? > > Which order are the type and checked attributes set in, in this case? (These answers haven't been verified in a debugger.) The HTML5 parser never reparses except when a speculation fails or when switching charset, but in that case the speculative tree op are thrown away before they reach the tree op executor. Thus, as far a the DOM is concerned, there's no reparsing other than switching the charset, but in that case the browsing context is renavigated. The parser sets attributes before inserting a node into the DOM (except when a second <html> or <body> tag causes attribute to be added to the first html and body elements). The attributes are set in the order they appear in the source. (So type first, checked second in this case.)
Given comment 11, I'm not sure how we're getting into this state, exactly...
OK. The first time we resolve style for the checkbox, it does NOT have the BF_CHECKED bit set. It does have BF_SHOULD_INIT_CHECKED and BF_PARSER_CREATING. It looks like DoneCreatingElement is happening after the append notification in this case. That seems wrong to me.
And in fact, the flush op is immediately before the eTreeOpDoneCreatingElement in the queue. So this is in fact a regression from bug 497861.
Blocks: 497861
So perhaps a fundamental issue is that getting our content lists up to date also involves frame construction... We need the former for form state restoration, we can't properly set checked state until after form state restoration, and we need the correct checked state by the time we do frame construction.
This patch swaps the order of the DoneAddingChildren and flush tree ops. It seems to make the css3.info test pass without regressing bug 497861. I tried to write a reftest regression test for this, but without copying the css3.info test, it isn't clear to me what exactly is needed to make this fail without the patch. Apparently, the failure isn't just :checked always failing to match without the patch...
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #418166 - Flags: review?(bzbarsky)
> It seems to make the css3.info test pass without regressing bug 497861. Why? The issue in bug 497861 was that GenerateStateKey was running before the flush, and GenerateStateKey is called from DoneCreatingElement, no? If this doesn't regress bug 497861, then I suspect our tests for that bug aren't good enough. > it isn't clear to me what exactly is needed to make this fail without the patch. As far as I can tell, just having an <input type="checkbox" checked> not in a form should do the trick. Does it not for you?
Attached patch Reftest — — Splinter Review
(In reply to comment #17) > > It seems to make the css3.info test pass without regressing bug 497861. > > Why? The issue in bug 497861 was that GenerateStateKey was running before the > flush, and GenerateStateKey is called from DoneCreatingElement, no? I reran the test to confirm that I didn't mistest the last time round. I don't have an explanation. I have to admit I don't have a clear understanding of what's going on the content tree side of things here. (I realize this isn't exactly a good basis for developing a patch.) > If this doesn't regress bug 497861, then I suspect our tests for that bug > aren't good enough. That's quite possible. Since the old parser passes Mochitest and the css3.info test, there has to be some sequence of calls that the parser can make to pass both tests. > > it isn't clear to me what exactly is needed to make this fail without the patch. > > As far as I can tell, just having an <input type="checkbox" checked> not in a > form should do the trick. Does it not for you? It works for me now if I keep the mouse pointer away from the input. (I guess I had the pointer in the wrong place or something yesterday.)
(The reftest is probably in the wrong place. parser/ could use a directory for reftests.)
> Since the old parser passes Mochitest and the css3.info test, there has to be > some sequence of calls that the parser can make to pass both tests. The old parser actually screws this up in some cases (testcase coming up). Specifically, any time it actually ends up flushing due to generating state keys under DoneCreatingElement there. The difference is that it only does this in _some_ cases, whereas the HTML5 parser inserted a 100% guaranteed flush. I'll attach a testcase that shows the bug with the old parser. Needs human interaction, unfortunately.
I think that the simplest solution here might in fact be to fix bug 497861 "the right way" (by moving the state restoration to a script runner). That approach would still be pretty annoying for the checkbox case, though.... sicking, any ideas?
Attachment #418166 - Flags: review?(bzbarsky)
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
Summary: [html5] test failure on css3.info selectors test with :checked{} → [HTML5] test failure on css3.info selectors test with :checked{}
Should this be treated as blocking the enablement of the HTML5 parser? (If yes, I could use help. If not, is this in the right component?)
> Should this be treated as blocking the enablement of the HTML5 parser? Sadly, I think so. It's a high-visibility standards-compliance test that the html5 parser fails... In terms of fixing this, perhaps just landing the lazy frame construction patch will do the trick?
Yeah, my build with the lazy frame construction patches applied and html5 enabled passes the test.
Depends on: lazyfc
So, following the landing of bug 502937, this appears fixed indeed. Tested with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100417 Minefield/3.7a5pre /rev/981132d2f5d6
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #418166 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: