Bug 33253 – Want access to <label> element associated with <input>

• Component: New Bugs
• Status: RESOLVED CONFIGURATION CHANGED

On Android, we want to use the text in the <label> element as a hint for an <input> element.  Although the information is freely available, I found myself copying labelForElement() in AccessibilityRenderObject.cpp.  Would it be possible to make this a public function?  That way we can call the same function, and if WebKit changes we will not need to make any changes.

Created attachment 45962 [details]
Patch making labelForElement a public static function in HTMLLabelElement

[WebKit Review Bot]

Attachment 45962 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 98, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 62, in main
    defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
AttributeError: 'module' object has no attribute 'ArgumentDefaults'

[Darin Adler]

Comment on attachment 45962 [details]
Patch making labelForElement a public static function in HTMLLabelElement

I'm not entirely sure this is a good change.

> +HTMLLabelElement* HTMLLabelElement::labelForElement(Element* element)
> +{
> +    RefPtr<NodeList> list = element->document()->getElementsByTagName("label");

I know you're just moving the above code, but I'll note that using the NodeList is a quite-inefficient way to iterate all the label elements -- it's the public DOM API for JavaScript use, but inside the engine it's not a great idea.

It would be straightforward to just iterate all the nodes in the document looking for label elements instead. It's also really strange that this function uses a node list created by getElementsByTagName but then still checks the tag name of each element in the list. That makes no sense.

You just moved the code, but it's not great code.

The only real value in having this function in the header would be knowing that the algorithm used is exactly the same one used by HTMLLabelElement itself. And yet I see no evidence that this is true. The function was used only in the accessibility code. It's inefficient to iterate an entire document, so using this function would typically not be encouraged.

And it's straightforward to implement this in terms of already existing public functions.

> +    unsigned len = list->length();

As long as you're moving the code, it would be good to use "length" instead of "len". We prefer words to abbreviations.

> Index: WebCore/html/HTMLLabelElement.h
> ===================================================================
> --- WebCore/html/HTMLLabelElement.h	(revision 52806)
> +++ WebCore/html/HTMLLabelElement.h	(working copy)
> @@ -56,6 +56,7 @@ public:
>  
>      void focus(bool restorePreviousSelection = true);
>  
> +    static HTMLLabelElement* labelForElement(Element* element);
>   private:
>      String m_formElementID;

There should be a blank line after it so this function isn't paragraphed with the private members below.

The argument name "element" should be omitted.

I assume you want to make this function public for some kind of Chromium use. Could you be more clear about the specifics? Typically, if there's a function that is not used anywhere in the WebKit source tree people will feel free to refactor so it's private again. So I think it would be best to add the use of the function at the same time you make it public.

review- for now

[Darin Adler]

Ah, not Chromium, Android.

It's OK to make this public, but it's also trivial to do this code in WebKit. I'm not entirely sure making this a public function is a good idea. If this was an important operation we'd probably want a more sophisticated approach so we didn't have to iterate the entire document each time.

[Darin Adler]

(In reply to comment #4)
> It's OK to make this public, but it's also trivial to do this code in WebKit.

In the WebKit layer rather than in WebCore.

[Anne van Kesteren]

This is now available through IDL.