96813 - LABEL element always treated as inline

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

After the fix for bug 47149, the LABEL element is always treated as an inline
element.  It can't be styled using 'display: block' or 'float', or 'position',
or if it can (I haven't tested much), it would break the fact that it's a LABEL.

If the event handling magic (which seems non DOM-compliant, too) and accesskey
code needed to make LABEL special were implemented in the content nodes rather
than the frames, then we could avoid this problem.

Comment 1

[mech]

Attachment: small testcase: xhtml1-strict with <label style="display: block">

Comment 2

[Andrew Chadwick]

Happens for me under Debian testing/unstable:

ii  galeon         0.12.4-0.1     Mozilla based web browser with GNOME look an
ii  mozilla-browse 0.9.5-4        Mozilla Web Browser - core and browser

With <http://bugzilla.mozilla.org/attachment.cgi?id=52711&action=view>, nothing
appears for me after the second <h2> ("with <label style="display: block">").

This bug completely breaks the many pages using <label> to improve their
accessibility (!), so suggest Severity: major.

Also see bug 100801.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We already know this bug happens.  There's no need to say it still happens.

Comment 4

[basic]

This needs to get fixed, it's forcing me to use <div> or <p> as a replacement on
some pages and sometimes use label:before,label:after{ content: "\A";
white-space: pre; } to get the same effect. Which does not always work the way I
want it. It works in IE and in Opera. How hard is it to fix this?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 126472 has been marked as a duplicate of this bug. ***

Comment 6

[bugzilla.mozilla.org]

just voted for this bug.

It's annoying to have to go back to using tables to layout forms just because
the styles on the LABELs don't work corectly :-((

if it at least would show the correct width as defined in the css ...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The width property doesn't apply to inline elements.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 1

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking the bug.

The patch I attached above moves the handling of the LABEL element entirely into
the content code.

The existing layout code for handling LABEL did two things:
 1. it had a hacked GetFrameForPoint that changed any event that was going to a
child of the label to be going to the label instead (think text nodes), unless
it was in an anchor or a form control
 2. it had code to register and unregister access keys

The patch:
 * removes nsLabelFrame, and the nsCSSFrameConstructor code that creates it (and
thus causes the frame to be constructed based on the 'display' property)
 * has the nsHTMLLabelElement register its access keys the same way
nsHTMLAnchorElement does.  I refactored the main function that does this into
nsGenericHTMLElement so they can share it, and copied the |SetDocument| member
function. I also copied the |SetAccessKey| method from nsHTMLAnchorElement so
that dynamic changes to access keys work.  (The |SetDocument| function, unlike
|SetAccessKey|, could be shared if we added yet another layer in the inheritance
chain for nsHTML{Anchor,Label}Element), but I thought it was too short a
function to bother.)   Finally, I changed the |SetAccessKey| and |GetAccessKey|
methods on both to do a GetAttr the same way NS_IMPL_STRING_ATTR does (why do we
use namespace HTML?  does it matter?).  I also cleaned up the RegUnRegAccessKey
function that I moved (reduced nested braces with early returns, mainly, and
cleaned up comments)  This handles (2) above.
 * Does a tiny bit of cleanup on nsIEventStateManager and removes the somewhat
confusing and basically useless frame parameter to RegisterAccessKey /
UnregisterAccessKey.  (I almost did a GetPrimaryFrameFor to pass in the right
frame.  All it was used for was so that callers could pass in *either* content
or frame, and if the caller passed a null content node, it would get the content
from the frame.  But there was only *one* caller taking advantage of that, so it
cost more code than it saved.)
 * Makes considerable modifications to nsHTMLLabelElement::HandleDOMEvent and
overrides nsIContent::SetFocus on nsHTMLLabelElement to replace the hacked
GetFrameForPoint ((1) above) and also handle the accesskey action better (by
giving focus to the "for" control for radio buttons and checkboxes, which the
code didn't used to do for labels, but does for other controls):
   + If we get an NS_FOCUS_CONTENT event, pass it straight on to the "for content."
   + If we get an NS_MOUSE_LEFT_CLICK event, call |SetFocus| on the "for
content" (this is the improvement on the previous code) and pass the event on to
the "for content", which we also used to do.  Now we have to do it in the
bubbling (new) or initial (as before) phase (but still not capturing) because we
lack the hacked GetFrameForPoint.  And since we handle bubbling-phase events
now, nsHTMLLabelElement needs a member variable to prevent infinite recursion
when the "for content" is inside the label content.

I think the HandleDOMEvent changes might cause some other differences in how DOM
Events users see the events (although we were running the event twice in some
cases before, too).  I might have also forgotten to mention something about
them.  They're somewhat complicated.  And I want to submit this comment before
the browser crashes. :-)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  I think I want to make the first check in the overridden SetDocument be
for a non-null mDocument rather than a null aDocument, in case we ever use
|importNode|.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 2

Tweaked SetDocument.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and, FWIW, I've been using attachment 46836 [details] (from bug 47149) as my main
testcase.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, really, SetAccessKey is the wrong place to handle the change, since it won't
catch |setAttribute|.  nsHTMLAnchorElement seems to do the right thing in one
place, but it checks for namespace ID of None rather than HTML.  I'm confused
about what our current bugginess of None vs. HTML is.  I also need to see
whether what nsHTMLAnchorElement::SetAttr does is the right thing or perhaps
whether we need a better general mechanism...

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 82363 [details] [diff] [review]
patch, v. 2

First, a matter of style: in general in content/html/content we are trying to
put braces around single-line if statements.  It makes it easier to distinguish
a block of code from a continuation of a line.	Please fix those up.


>Index: content/html/content/src/nsGenericHTMLElement.cpp

>+nsresult nsGenericHTMLElement::RegUnRegAccessKey(PRBool aDoReg)
>+{
>+  nsresult rv;
>+
>+  // first check to see if we have an access key
>+  nsAutoString accessKey;
>+  rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::accesskey, accessKey);
>+  if (NS_FAILED(rv))
>+    return rv;

Please use NS_ENSURE_SUCCESS(rv, rv) in places like this.  There are a few
others in the patch.


>Index: content/html/content/src/nsHTMLAnchorElement.cpp

> NS_IMETHODIMP
> nsHTMLAnchorElement::GetAccessKey(nsAString& aValue)
> {
>-  NS_STATIC_CAST(nsIHTMLContent *, this)->GetAttr(kNameSpaceID_None,
>-                                                  nsHTMLAtoms::accesskey,
>-                                                  aValue);
>+  GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::accesskey, aValue);

This should remain kNameSpaceID_None, IIRC we are supposed to use that
namespace for all attribute gets and sets.  Same with other places in this
file.

>Index: content/html/content/src/nsHTMLLabelElement.cpp

>-NS_IMPL_STRING_ATTR(nsHTMLLabelElement, AccessKey, accesskey)
>+//NS_IMPL_STRING_ATTR(nsHTMLLabelElement, AccessKey, accesskey)

This should be removed rather than commented.

> NS_IMETHODIMP
>+nsHTMLLabelElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
>+                                PRBool aCompileEventHandlers)
>...
>+  nsresult rv = nsGenericHTMLContainerElement::SetDocument(aDocument, aDeep,
>+                                                        aCompileEventHandlers);

You need to call nsGenericHTMLContainerFormElement::SetDocument instead;
otherwise, the element is not going to be properly added / removed from
form.elements.

>+        rv = content->HandleDOMEvent(aPresContext, aEvent, aDOMEvent,
>+                                     aFlags, aEventStatus);

It seems to me we need to fudge the target here.  This was broken in the old
code too, so you're free to ignore (I'll file a bug if so); but it may be as
simple as SetOriginalTarget() and SetTarget().	I'll check IE's behavior when I
reboot into Windows; I suspect it sets srcTarget and toTarget.


> Index: layout/html/forms/src/nsLabelFrame.cpp

>-    nsCOMPtr<nsIFormControlFrame> controlFrame = do_QueryInterface(*aFrame);
>-    if (!controlFrame) {
>-      // if the hit frame isn't a form control then
>-      // check to see if it is an anchor
>-

The code to check if it's an anchor does not seem to be in the new code.  Nor
are the visibility checks, but presumably the standard focus mechanism will
take care of that.


r=jkeiser with that stuff, and with catching SetAttr()

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> This should remain kNameSpaceID_None, IIRC we are supposed to use that
> namespace for all attribute gets and sets.  Same with other places in this
> file.

Then somebody really needs to change NS_IMPL_STRING_ATTR.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> The code to check if it's an anchor does not seem to be in the new code.  Nor
> are the visibility checks, but presumably the standard focus mechanism will
> take care of that.

That isn't needed anymore.  |nsLabelFrame::GetFrameForPoint| "fixed-up" the
frame for any child of a label frame (normally a text node) that wasn't a form
control or in an anchor so that it would look like the click landed directly on
the LABEL element itself (which is wrong).  This was needed in the old world
because the old version of |nsHTMLLabelElement::HandleDOMEvent| only did its
magic stuff given (NS_EVENT_FLAG_INIT & aFlags), i.e., that the event was at its
target.

Comment 17

[John Keiser (jkeiser)]

> Then somebody really needs to change NS_IMPL_STRING_ATTR

Yes, somebody needs to clean up a lot of the HTML namespace horkage, there's a 
bug on it.  Anyway, if JST says it's OK or desirable to HTML namespace I'll be 
fine with it; I know he's told me to use None before.

As to the .target thing:

It turns out IE sets .srcElement to the input (which seems wrong in itself) 
but that isn't really the point.  We need to set .target properly when we 
redirect an event like this.  Again, you don't have to deal with it here if 
you don't want to.  If you want to, it should be a simple matter of 
setting .target.  (I think you're right, we should leave .originalTarget 
alone.)  While it would be nice if there were a "source" property (I can't 
find one) to set to the label element, I don't think it's as important as the 
event having the proper state.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FYI: bug 134278 is about changing all errorous kNameSpaceID_HTML to 
kNameSpaceID_None and dropping the code that allows the errorous use.

Comment 19

[Fabian Guisset]

Comment on attachment 82363 [details] [diff] [review]
patch, v. 2

>+nsresult nsGenericHTMLElement::RegUnRegAccessKey(PRBool aDoReg)
>+{
>+  nsresult rv;
>+
>+  // first check to see if we have an access key
>+  nsAutoString accessKey;
>+  rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::accesskey, accessKey);

nsresult rv = GetAttr(....);

>+  if (NS_FAILED(rv))
>+    return rv;

NS_ENSURE_SUCCESS(rv, rv)   is the standard macro in the DOM

>+already_AddRefed<nsIContent>
>+nsHTMLLabelElement::GetForContent()
>+{
>+  nsresult rv;
>+
>+  // Get the element that this label is for
>+  nsAutoString elementId;
>+  rv = GetHtmlFor(elementId);

nsresult rv = GetHtmlFor(elementId);

Great cleanup, thanks!

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 3

This addresses most of the comments, I think.  I didn't do any event
retargetting yet (I should file a separate bug).  I also changed the way I deal
with namespaces a little bit, again, since the current code is quite fragile. 
In particular, the code that traps |SetAttr| really needs to check both
namespaces.  (nsHTMLAnchorElement::SetHref has a workaround for this, but I
preferred to put all the logic in the SetAttr / UnsetAttr and use
NS_IMPL_STRING_ATTR for AccessKey, despite its defects.  (I also switched
nsHTMLAnchorElement to doing that for accesskey, but left the href code alone
pending the attribute namespace cleanup.)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 144634 has been marked as a duplicate of this bug. ***

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 4

This is merged with jst's changes.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase showing SetHref change is OK

This testcase demonstrates that the |SetHref| change is OK -- that code should
have been removed by jst's patch, since it was only needed because the code in
SetAttr was conditioned on kNameSpaceID_None, while SetHref used to pass in
kNameSpaceID_HTML.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 84383 [details] [diff] [review]
patch, v. 4

sr=jst

Looks like you don't need nsHTMLLabelElement::SetAttr() (the one that takes a
nsINodeInfo argument), the baseclass will do the right thing AFAICT.

Comment 25

[John Keiser (jkeiser)]

Comment on attachment 84383 [details] [diff] [review]
patch, v. 4

r=jkeiser

Comment 26

[John Keiser (jkeiser)]

Comment on attachment 84383 [details] [diff] [review]
patch, v. 4

collision

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Looks like you don't need nsHTMLLabelElement::SetAttr() (the one that takes a
> nsINodeInfo argument), the baseclass will do the right thing AFAICT.

Not overriding would make it impossible to call that version of SetAttr within
the class without a cast.  Do we care?  If I undo that, should I make the same
change to nsHTMLAnchorElement?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And I suspect it would lead to a bunch of method-hiding warnings.)

Comment 29

[Johnny Stenback (:jst)]

Nah, don't worry about it then.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2002-05-21 17:14 PDT.  Filed bug 146066 on the event target issue.

Comment 31

[Aaron Leventhal]

Attachment: Regressed HTML example, checkbox won't check

When using a checkbox with a label, the checkbox will no longer check.
I'm guessing that's from this checkin.

<label for="checkbox1">
  <input id="checkbox1" type="checkbox"/> my checkbox </label>
</label>

Comment 32

[Aaron Leventhal]

Clarification -- if you click on the checkbox or use spacebar it won't check. If
you click on the label it will check.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for regression

This fixes the regression.  The problem was that it was getting two click
events.  I should have tested more cases with the for content inside the
label...

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for regression, with better comment

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 84630 [details] [diff] [review]
fix for regression, with better comment

sr=jst

Comment 36

[John Keiser (jkeiser)]

Comment on attachment 84630 [details] [diff] [review]
fix for regression, with better comment

r=jkeiser

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regression fix checked in 2002-05-22 17:23 PDT.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 164004 has been marked as a duplicate of this bug. ***

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Nominating for 1.0.2 since this missed the 1.0 branch (and bug 28657 which
supposedly was fixed by this patch is nominated).  dbaron, can you say if you
think this patch would work for the 1.0 branch, or would it need others?  If
this doesn't make 1.0.2, we'll roll it into branch after that.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also need to pull in the patches from:  bug 148760.

Other unmarked duplicates of this bug include:  (bug 100801), bug 134869, (bug
53259), bug 28657 (also fixed by this patch, although not really the same bug)

Regressions suspected to be related, but not: bug 148249

A still-unfixed regression from this bug: bug 158043.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patches merged to 1.0 branch

This contains the following, merged to the 1.0 branch:
 * attachment 84383 [details] [diff] [review] from this bug, with namespace changes in the anchor element

   and label element SetAttribute methods that revert to what I had in
attachment
   82530 [details] [diff] [review], before jst landed his namespace cleanup.
 * attachment 84630 [details] [diff] [review] from this bug
 * attachment 86086 [details] [diff] [review] from bug 148760
 * attachment 98736 [details] [diff] [review] from bug 158043 (not checked in yet to trunk, but trivial)

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 98738 [details] [diff] [review]
patches merged to 1.0 branch

sr=jst

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 98738 [details] [diff] [review]
patches merged to 1.0 branch

For the record, the email that I sent jst about this patch was:

The only significant changes between what landed on the trunk and what's
in this patch are six changes (3 each in nsHTMLAnchorElement and
nsHTMLLabelElement) of the form (diff of diff):

< +  if (aName == nsHTMLAtoms::accesskey && kNameSpaceID_None == aNameSpaceID)
{---
> +  if (aName == nsHTMLAtoms::accesskey &&
> +      (kNameSpaceID_None == aNameSpaceID ||
> +       kNameSpaceID_HTML == aNameSpaceID)) {

Does this seem like the right thing to do?  The patch does change manual
implementations of {Set,Get}AccessKey to NS_IMPL_STRING_ATTR, which
changes the namespace ID being used for the accessKey property (from
None to HTML).	This seems like it should be fine as long as:
 * we're checking both namespaces in the SetAttr/UnsetAttr functions
 * the namespace ID isn't actually stored, so a GetAttr with either
   namespace ID will retrieve the attribute.
My memory is that both of these were true before the HTML attribute
namespace cleanup that you landed, but I could be wrong.  Any thoughts?

Comment 44

[Brendan Eich [:brendan]]

Comment on attachment 98738 [details] [diff] [review]
patches merged to 1.0 branch

a=[e-mail] on behalf of drivers for the 1.0 branch.

Is there any point in getting re-review (r=)?

/be

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 98738 [details] [diff] [review]
patches merged to 1.0 branch

r=bzbarsky

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

a=[e-mail] refresh of a= for 1.0 branch

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_0_BRANCH, 2002-10-02 17:20 PDT.

Comment 48

[Bill Mason]

Verified fixed in 2002100306-1.0 branch PC/Win98.  From comment 40, also
verified that this fixes bug 100801, bug 53259, and bug 28657.  Bug 134869
testcase still looks wrong.

As the person who asked for this so I could see bug 28657 fixed on the branch,
thanks to everyone who put time into it.  Much appreciated.

Comment 49

[Madhur Bhatia]

changing KW from fixed1.0.2 to verified1.0.2
verified on win2000 ----- 2002-10-03-08-1.0 branch build

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 178439 has been marked as a duplicate of this bug. ***