180512 - Textboxes do not support context attribute overriding default context menu

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Alex Vincent [:WeirdAl]]

The XUL textbox widget's bindings do not inherit the context menu from the
textbox element.  This makes life a little difficult for situations when we wish
to override the default menu.

Workaround involves textbox.inputField.parentNode.setAttribute("context",
textbox.getAttribute("context"));

Patch is really simple, but I left it at home.  Basically:

-      <xul:hbox class="textbox-input-box" flex="1">
+      <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context">

...

-     <xul:hbox class="textbox-input-box" flex="1">
+     <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context">

...

-    <content context="_child">
+    <content context="_child" inherits="context">

Comment 1

[James Ross]

Attachment: diff -u version of comments.

Comment 2

[Alex Vincent [:WeirdAl]]

That'll do it... thanks to sp3k for making me look at LXR for more recent
changes, so we don't leave out the timed-textbox fix.

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 106530 [details] [diff] [review]
diff -u version of comments.

requesting review.  This patch hasn't bitrotted yet.

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 106530 [details] [diff] [review]
diff -u version of comments.

>@@ -100,11 +100,11 @@
>     </handlers>    
>   </binding>
> 
>   <binding id="timed-textbox" extends="chrome://global/content/bindings/textbox.xml#textbox">
>     <content>
>-      <xul:hbox class="textbox-input-box" flex="1">
>+      <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context">
>         <html:input class="textbox-input" flex="1" anonid="input"
>                     xbl:inherits="onfocus,onblur,value,type,maxlength,disabled,size,readonly,tabindex"/>
>       </xul:hbox>
>     </content>
>     <implementation>

Oops, we need to remove this part.  Bug 197486 removed the content element for
the binding.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: Testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106530 [details] [diff] [review]
diff -u version of comments.

This needs r or sr from a module owner or peer for this code...

Comment 7

[jag (Peter Annema)]

New patch synch'ed up with trunk, please. And make sure it works for all three
textbox types.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: More comprehensive testcase

This testcase covers:

<textbox/>
<textbox multiline="true"/>
<textbox type="timed"/>
<textbox type="autocomplete"/>

chrome://global/content/xul.css indicates there are no other textbox bindings.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, including autocomplete

jag:  good call making me check; in the process I forgot about autocomplete.

Comment 10

[Alex Vincent [:WeirdAl]]

Comment on attachment 125876 [details] [diff] [review]
patch, including autocomplete

Updated patch to reflect:
(1) removed patch code for timed textbox, per comment 4
(2) included patch code for chrome://global/content/autocomplete.xml

Comment 11

[jag (Peter Annema)]

Comment on attachment 125876 [details] [diff] [review]
patch, including autocomplete

   <binding id="input-box">
-    <content context="_child">
+    <content context="_child" inherits="context">

Shouldn't that be xbl:inherits? Right now, that attribute is getting ignored,
which is I think exactly what we want here, since the context="_child" won't
(or shouldn't) overwrite the context specified (or in this case inherited onto)
the content it's getting bound to.

Comment 12

[Alex Vincent [:WeirdAl]]

http://www.mozilla.org/projects/xbl/xbl.html#anonymous-attributes

I disagree with that; the element the inherits attribute applies to in this case
is already in the XBL namespace.  The spec itself does not explicitly require
the prefix.

The attribute is not getting ignored.  Interestingly enough, if you look at it
in DOM Inspector, apparently the xbl prefix applies anyway...

More significantly, though, I tried running my patch without the input-box
binding being altered, and to my surprise, the context menu was still replaced!

More to the point, adding in the context attribute to the other bindings renders
the need for the class attribute of those bindings' <xul:hbox/> elements
obsolete.  After a little research, I discovered <xul:hbox
class="textbox-input-box"/> is there specifically to create the context menu,
and that's all.

Comment 13

[Alex Vincent [:WeirdAl]]

Attachment: patch, without altering <binding id="textbox-input-box"/>

same as attachment 125876 [details] [diff] [review], minus the code which jag denied sr= on.

Comment 14

[jag (Peter Annema)]

> http://www.mozilla.org/projects/xbl/xbl.html#anonymous-attributes
>
> I disagree with that; the element the inherits attribute applies to in this
> case is already in the XBL namespace.  The spec itself does not explicitly
> require the prefix.

The attribute isn't in the xbl namespace:
http://www.w3.org/TR/xml-names11/#defaulting

"A default namespace declaration applies to to all unprefixed element names
within its scope. Default namespace declarations do not apply directly to
attribute names; the interpretation of unprefixed attributes is determined by
the element on which they appear."

For XBL we've chosen to require the explicit XBL namespace prefixing for
|inherits| (the xbl spec needs to be corrected to make that clear).

http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLPrototypeBinding.cpp#988

> The attribute is not getting ignored.  Interestingly enough, if you look at 
> it in DOM Inspector, apparently the xbl prefix applies anyway...

I'll take a look. DOM Inspector might be getting this wrong.

> More significantly, though, I tried running my patch without the input-box
> binding being altered, and to my surprise, the context menu was still
> replaced!

I'm not surprised, see comment 11. Or let me try to explain it differently:
what's happening is that the |context="_child"| on the "input-box" binding only
gets copied into the element it's bound to if it doesn't have that attribute
already. Because you've added |xbl:inherits="context"| to the <hbox>, the
attribute, if specified on the <textbox>, will exist and the default value for
it ("_child") won't get copied over from the "input-box" binding.

> More to the point, adding in the context attribute to the other bindings
> renders the need for the class attribute of those bindings' <xul:hbox/>
> elements obsolete.  After a little research, I discovered <xul:hbox
> class="textbox-input-box"/> is there specifically to create the context menu,
> and that's all.

You still need the "input-box" binding bound to those <hbox>en. If you remove
the class="textbox-input-box", you won't have a default context menu anymore
(neither the "_child" default nor the actual menu items).

Comment 15

[jag (Peter Annema)]

Comment on attachment 125978 [details] [diff] [review]
patch, without altering <binding id="textbox-input-box"/>

sr=jag

Comment 16

[jag (Peter Annema)]

Checked in.

Comment 17

[[e-mail]]

So, do we need the same thing for editable menulists?

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: patch for <menulist editable="true"/>

Neil: good of you to point that out.  Technically, that would merit a separate
bug, but it'd be a waste of everyone's time and Bugzilla's resources.  The
patch is trivial anyway.

Comment 19

[jag (Peter Annema)]

Comment on attachment 126065 [details] [diff] [review]
patch for <menulist editable="true"/>

sr=jag

Comment 20

[[e-mail]]

Fix checked in.

Comment 21

[Alex Vincent [:WeirdAl]]

FYI, bug 312869 is a followup to this (because this bug was actually a bad idea).