565611 - "ASSERTION: How did that happen?: 'form == aThisForm'" with <legend>

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: How did that happen?: 'form == aThisForm', file /home/jruderman/mozilla-central/content/html/content/src/nsHTMLFormElement.cpp, line 513

I think this is a very recent regression.

Comment 1

[Jesse Ruderman]

The assertion happens when you close the window containing the testcase, or something like that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Presumably a regression from bug 555567.

Comment 3

[Mounir Lamouri (:mounir)]

I confirm that's a regression from bug 555567. mForm is set to the parent form but GetForm() return a the form of the parent fieldset element. If there is no fieldset element, mForm is set to the parent but GetForm() returns null.

Actually, nsHTMLLegendElement shouldn't be a nsGenericHTMLFormElement. It's not submittable, not labelable, not listed, not resettable. It's more like an option element with a form attribute.

Thank you for your report Jesse :)

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

With this patch, nsHTMLLegendElement is not a form element anymore.
The behavior did not changed so I don't see any test.

Boris, let me know if you have not enough time to review this patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445223 [details] [diff] [review]
Patch v1

r=bzbarsky

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, hold on.  Did you check all the eHTML_FORM_CONTROL consumers?  In particular, can <legend> ever be focusable?  What happens if a <label> is labeling a legend?

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> Actually, hold on.  Did you check all the eHTML_FORM_CONTROL consumers?  In
> particular, can <legend> ever be focusable?  What happens if a <label> is
> labeling a legend?

<legend> is not labelable nor focusable. The user should not be able to interact with <legend>. By the way, maybe be we could remove ::Focus and ::PerformAccessKey from nsHTMLLegend ?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> <legend> is not labelable nor focusable.

Everything can be focusable.  Just set a tabindex.

And it seems like <legend> has a pretty consciously chosen ::Focus behavior when it's not focusable, no?  Why would you want to break that?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> > <legend> is not labelable nor focusable.
> 
> Everything can be focusable.  Just set a tabindex.
> 
> And it seems like <legend> has a pretty consciously chosen ::Focus behavior
> when it's not focusable, no?  Why would you want to break that?

Indeed, everything is focusable with tabindex, I forget that.
I was reading bug 81481 and realized ::Focus has a reason to exist. It's quite hard to find the corresponding HTML5 specification by the way.

Anyway, that means the current patch is correct, right ?

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> Actually, hold on.  Did you check all the eHTML_FORM_CONTROL consumers?  In
> particular, can <legend> ever be focusable?  What happens if a <label> is
> labeling a legend?

I'm assuming everything is okay regarding the focus (as far as I've checked/tested). For the labeling, as legend is not labelable, it's okay. Can you confirm that, Boris ?

Comment 11

[Boris Zbarsky [:bzbarsky]]

So why is legend not labelable?  Would it make sense for it to be?  As in, should we ask for a spec change?  What makes the most sense for web authors?

Note that in our current code it's labelable but the label's action doesn't call focus() and the focus manager doesn't seem to have the label special-case.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> So why is legend not labelable?  Would it make sense for it to be?  As in,
> should we ask for a spec change?  What makes the most sense for web authors?
> 
> Note that in our current code it's labelable but the label's action doesn't
> call focus() and the focus manager doesn't seem to have the label special-case.

I don't think it would make sense to have a labelable <legend> as <legend> is already a kind of "special label" for the <fieldset>. I don't see what information the label could add to the legend.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  OK, good point.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so did you audit the other HTML_FORM_CONTROL consumers like I asked you to?  At least for the nsDOMClassInfo one it looks like the patch you attached will give the wrong behavior, no?

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #14)
> OK, so did you audit the other HTML_FORM_CONTROL consumers like I asked you to?
>  At least for the nsDOMClassInfo one it looks like the patch you attached will
> give the wrong behavior, no?

I checked or tested everything. For nsDOMClassInfo, I do not really get what could be wrong. |WrapNative| wouldn't be called anymore for <legend> and the parent would be the document instead of the form but is that an issue ?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I checked or tested everything.

Do we have existing tests for those codepaths?  If not, how did you test?  Is the new behavior definitely better in all cases?  I'd appreciate a quick summary of why the change is ok or an improvement at all the callsites so I don't have to go through them all myself.

> but is that an issue ?

I'd think so.  Consider this testcase:

  <form>
    <input name="x">
    <fieldset>
      <legend onclick="try{ alert(x); } catch(e) { alert(e); }">
        Click me and see what happens
      </legend>
    </fieldset>
  </form>

At the moment, behavior on this testcase is interoperable at least in Chrome, Safari, Opera, and Firefox.  Does your patch not break it?

Comment 17

[Mounir Lamouri (:mounir)]

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp#2271
http://mxr.mozilla.org/mozilla-central/source/embedding/browser/gtk/src/EmbedContextMenuInfo.cpp#186
-> obviously doesn't change nothing

http://mxr.mozilla.org/mozilla-central/source/embedding/components/find/src/nsFind.cpp#342 -> <legend> isn't a text control so it only make us leave the function sooner

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1928 -> there is no caret in <legend>

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLLabelElement.cpp#399 -> <legend> isn't labelable

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsNodeUtils.cpp#244 -> <legend> isn't linked to the form

http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#2863 is used here http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#2870 so we probably don't care for <legend>

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2367 -> the <legend> frame is a text frame so it will not change anything

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp#418 -> for this one, I've put a breakpoint to see if we were going through line 418 with a <legend> and i didn't found a case where we were. In other words, |IsVisualMode()| was always returning false. I'm wondering if it could returns 'true' and break things sometimes...

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2074 -> focus seems to work as expected for <legend> so I suppose it isn't breaking anything here but I wasn't able to test.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: nsDOMClassInfo patch

Does this patch would be correct for you Boris ? (to fix the scope issue)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> there is no caret in <legend>

There sure can be in caret browsing mode!  That said, we don't want to treat legend as a leaf, so this part looks ok.

> the <legend> frame is a text frame so it will not change anything

I don't follow this (and note , the legend frame is a block frame, not a text frame).  That said, excluding legend here doesn't necessarily make sense, so we're probably ok.

> I'm wondering if it could returns 'true' and break things sometimes...

It can certainly return true; it depends on the charset the page is in.  That said, treating legend as visual-only doesn't make sense, so we're good here.

The focus manager code seems to be making sure we don't step out of form controls when moving focus, but stepping out of legend is desirable.  Good.

Sounds like classinfo is the only problem child.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445945 [details] [diff] [review]
nsDOMClassInfo patch

Yeah, that should work.  Fix the indentation and s/it's/its/ and s/form form/form/, and looks fine.

We should really deCOM nsIFormControl; file a bug on that if there isn't one already?

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Do you want/need to re-review the patch Boris ?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 446058 [details] [diff] [review]
Patch v2

The indentation on the else if condition in nsDOMClassInfo.cpp is still off.  Other than that looks good.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

r=bzbarsky

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 446383 [details] [diff] [review]
Patch v2.1

I'm hoping this doesn't affect to createElement performance too badly.

Comment 25

[Mounir Lamouri (:mounir)]

r=bz sr=smaug

Comment 26

[Justin Wood (:Callek)]

I'm pushing this to try in just a bit; I fear it might cause some assertion-related orange though, and might need an update. I'll post here if so.

Comment 27

[Justin Wood (:Callek)]

Pushed as http://hg.mozilla.org/mozilla-central/rev/0c87f407797d

Had a small hunk to update, I just did it after getting Mounir's once-over as if it was a bustage fix.