309054 - Remove nsGenericHTMLElement::GetAttr

Status: VERIFIED FIXED

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

Description

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

nsGenericHTMLElement::GetAttr could be removed if we make sure
that all its callers handle NS_CONTENT_ATTR_NO_VALUE and NS_CONTENT_ATTR_HAS_VALUE
properly. Atm nsGenericHTMLElement::GetAttr doesn't return 
NS_CONTENT_ATTR_NO_VALUE even if the return value is empty.

Comment 1

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

I'm not sure what to do with these
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLAreaAccessible.cpp#73
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLAreaAccessible.cpp#75
Have those ever worked?

Comment 2

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

Attachment: v1

Basically this converts 
NS_CONTENT_ATTR_HAS_VALUE == nsGenericHTMLElement::GetAttr() expressions
to NS_CONTENT_ATTR_NOT_THERE != nsGenericElement::GetAttr() 
and removes nsGenericHTMLElement::GetAttr()
Also few other changes.

Aaron, could you check that changes to accessible/ are correct?

If/when this gets checked in, there are some cases 
when the attrValue.IsEmpty() can be changed to use
the return value of the GetAttr(), which after this fix
will always return either
NS_CONTENT_ATTR_NOT_THERE, NS_CONTENT_ATTR_NO_VALUE( == IsEmpty()) or
NS_CONTENT_ATTR_HAS_VALUE.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196592 [details] [diff] [review]
v1

>Index: accessible/src/html/nsHTMLFormControlAccessible.cpp
>Index: accessible/src/html/nsHTMLImageAccessible.cpp

This part really needs review from Aaron.

>Index: content/base/src/nsContentUtils.cpp
>@@ -1690,18 +1690,18 @@ nsContentUtils::LookupNamespaceURI(nsICo
>+    if (content->GetAttr(kNameSpaceID_XMLNS, name, aNamespaceURI) !=
>+        NS_CONTENT_ATTR_NOT_THERE)

This makes this function inconsistent with nsNode3Tearoff::LookupPrefix.  Is
that really desirable?

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

Don't you need to fix GetURIAttr?

Also, it looks like nsGenericHTMLElement::RegUnRegAccessKey() should remove its
IsEmpty() check.

Mental note: nsHTMLAnchorElement::GetTarget is not consistent with the
GetBaseTarget calls in nsGenericHTMLElement::HandleDOMEventForAnchors....
probably worth a separate bug.

Another mental note: nsHTMLButtonElement::SubmitNamesValues probably wants to
not submit if the name is empty.  Again, a separate bug, please.  Same for
other SubmitNamesValues impls, maybe (image inputs excluded).

>Index: content/html/document/src/nsHTMLContentSink.cpp

>@@ -3682,22 +3682,22 @@ HTMLContentSink::ProcessBASETag(const ns

I think the checks against NS_CONTENT_ATTR_HAS_VALUE in this method were
correct.  Please undo that change, but make sure jst OKs that?

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp

Same here.

>Index: content/xml/document/src/nsXMLContentSink.cpp

Same here as for the HTML sink.

>Index: content/xul/document/src/nsXULDocument.cpp

>@@ -4066,22 +4064,22 @@ nsXULDocument::FindBroadcaster(nsIConten
>-        if ((rv != NS_CONTENT_ATTR_HAS_VALUE) || (aBroadcasterID.IsEmpty())) {
>+        if (rv == NS_CONTENT_ATTR_NOT_THERE) {

No, I think you want to test != NS_CONTENT_ATTR_HAS_VALUE here....

>Index: layout/forms/nsButtonFrameRenderer.cpp

> PRBool
> nsButtonFrameRenderer::isDisabled() 
> {
>   // get the content
>   nsAutoString value;
>-  if (NS_CONTENT_ATTR_HAS_VALUE ==
>+  if (NS_CONTENT_ATTR_NOT_THERE !=
>       mFrame->GetContent()->GetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, value))

How about making this function do:

  return mFrame->GetContent()->HasAttr(kNameSpaceID_None,
				       nsHTMLAtoms::disabled);

instead?

nsGfxButtonControlFrame::AttributeChanged seems to have a useless rv check for
a GetAttr() call.  Followup?

nsTextControlFrame::CreateAnonymousContent seems to have some GetAttr calls
that could be HasAttr ones instead.  Followup?

>Index: layout/style/nsCSSStyleSheet.cpp

>-        if (attrState != NS_CONTENT_ATTR_HAS_VALUE &&
>+        if (attrState == NS_CONTENT_ATTR_NOT_THERE &&

Leave this as != NS_CONTENT_ATTR_HAS_VALUE.

>Index: widget/src/gtk/nsNativeThemeGTK.cpp
>-            if (res == NS_CONTENT_ATTR_NO_VALUE ||
>-               (res != NS_CONTENT_ATTR_NOT_THERE && attr.IsEmpty()))
>+            if (res != NS_CONTENT_ATTR_HAS_VALUE)
>               *aWidgetFlags = FALSE;
>             else
>               *aWidgetFlags = attr.EqualsIgnoreCase("true");

Why is this change correct?  Fwiw, it looks to me like the original logic is
just bogus and that this should just use AttrValueIs().

>Index: widget/src/gtk2/nsNativeThemeGTK.cpp

Same.

>Index: widget/src/xpwidgets/nsNativeTheme.cpp
> nsNativeTheme::GetAttr(nsIFrame* aFrame, nsIAtom* aAtom, nsAString& >-  return ((res != NS_CONTENT_ATTR_NOT_THERE) &&
>-	  !(res != NS_CONTENT_ATTR_NO_VALUE && attrValue.IsEmpty()));
>+  return (res == NS_CONTENT_ATTR_HAS_VALUE);

Again, why is this correct?

You might want bryner to take a look at the various native theme stuff..

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196592 [details] [diff] [review]
v1

Bryner, could you look at the native theme stuff?

Comment 5

[Aaron Leventhal]

Comment on attachment 196592 [details] [diff] [review]
v1

I suppose it makes more sense for accessibility to explicity allow the author
to set the accessible name to nothing, so checking for
NS_CONTENT_ATTR_NOT_THERE makes sense as opposed to checking whether there is a
value or not. In that case we should also fix nsHTMLAreaAccessible to use
NS_CONTENT_ATTR_NOT_THERE as well. r+=aaronlev with that change.

Comment 6

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

(In reply to comment #3)
> Another mental note: nsHTMLButtonElement::SubmitNamesValues probably wants to
> not submit if the name is empty.  Again, a separate bug, please.  Same for
> other SubmitNamesValues impls, maybe (image inputs excluded).

IIRC we actually do want to submit empty-named forms. jkeiser did a fair amount
of testing on this and there were pages out there that depended on the current
behaviour. Probably because servers rely on getting the name/value pairs by index.

Comment 7

[Boris Zbarsky [:bzbarsky]]

My point was that nsHTMLInputElement only submits if it's an image or the name
is nonempty.  See
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#2238

Which leads me to suspect that the less-often-used form controls that sumbmit on
empty names might be buggy...  That is, why should a <button type="button">
behave differently from an <input type="button">?

That said, I agree that this is something worth testing to see what IE does...

Comment 8

[Hixie (not reading bugmail)]

I don't understand.

Of <button type="button"> and <input type="button"> neither should ever submit.
Am I missing something?

Comment 9

[Boris Zbarsky [:bzbarsky]]

s/type="button"/type="submit"/ if you prefer.  The issue remains.

Comment 10

[Martijn Wargers (dead)]

Ok, with testing this: http://wargers.org/mozilla/309054_getattr.htm
IE6, isn't sending anything when submitting this (only an '?' is added to the url)
Mozilla seems to send <input type="text">, <select> and <textarea>.

Comment 11

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

I've merged the changes in here with the patch in bug 311827.

Comment 12

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

Fixed by Bug 311827

Comment 13

[Frank Wein [:mcsmurf]]

So the patch here is obsolete/this bug here has been fixed by Bug 311827? If yes, cancel review, mark bug as dep and resolve as fixed?

Comment 14

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

It's not really a dup, but this bug is fixed, so resolution is right as is.

Marking verified.