312642 - expose clickSelectsAll property in textbox.xml

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[:Gavin Sharp [email: [e-mail]]]

Currently, Firefox (search bar, location bar) and Help viewer implement the
clickSelectsAll behavior on their own. I propose implementing the
clickSelectsAll behavior in the textbox widget, controlled by a property, off by
default. This way anyone who wants to check a pref can just set the property on
the textbox instead of implementing their own by overriding focus, onclick,
onmouseup, etc.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: binding changes

I think this works, but I haven't tested extensively.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 3

[[e-mail]]

Comment on attachment 203686 [details] [diff] [review]
patch

>+                onset="if (val) this.setAttribute('clickSelectsAll', 'true');
>+                       else this.removeAttribute('clickSelectsAll'); return !!val;" />
You should just return val here.

>+            if (this.mIgnoreFocus)
>+              this.mIgnoreFocus = false;
The problem here is that this handler is a capturing handler so may see two focus events if the textbox is scriptably focused. Putting this block in the else of the focusedElement test might solve that, but I haven't tried it.

>+          if (this.mIgnoreClick) {
I think this should be if (!this.mIgnoreClick)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)

Ok, I'll return val. I figured !!val would be more useful, but I guess it doesn't need to be explicitly boolean.

> >+            if (this.mIgnoreFocus)
> >+              this.mIgnoreFocus = false;
> The problem here is that this handler is a capturing handler so may see two
> focus events if the textbox is scriptably focused. Putting this block in the
> else of the focusedElement test might solve that, but I haven't tried it.

Hmm, I'll look into this further.

> >+          if (this.mIgnoreClick) {
> I think this should be if (!this.mIgnoreClick)

I could change this to this.hasAttribute("focused"), to be clearer, I guess? I make the assignment to ignoreclick right above that.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> >+          if (this.mIgnoreClick) {
> I think this should be if (!this.mIgnoreClick)

Oops, you're right.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2

Comment 8

[[e-mail]]

Comment on attachment 203732 [details] [diff] [review]
patch v2

>+                onget="return (this.getAttribute('clickSelectsAll') == 'true');"
Nit: return doesn't require brackets.

>       <method name="setSelectionRange">
Nit: you added the clickSelectsAll property between the selection properties and method. Maybe put it with the other mapped attribute properties?

>+            } else {
>+              if (this.mIgnoreFocus)
>+                this.mIgnoreFocus = false;
>+              else if (this.clickSelectsAll)
>+                this.inputField.select();
>+            }
Nit: braces unnecessary for a single statement (both then and else cases, now that I notice).

>+      <handler event="mousedown" phase="capturing">
...
>+      <handler event="click" phase="capturing">
Nit: These might not actually need to be capturing (focus is special).

I noticed that on certain existing textboxes e.g. the address book search that as well as the click to select behaviour it also selects when you press the return key - do you think that's something to consider adding too?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: nits addressed

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/textbox.xml
new revision: 1.22; previous revision: 1.21
mozilla/xpfe/global/resources/content/bindings/textbox.xml
new revision: 1.33; previous revision: 1.32

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> >+      <handler event="mousedown" phase="capturing">
> ...
> >+      <handler event="click" phase="capturing">
> Nit: These might not actually need to be capturing (focus is special).

I didn't address this nit, but I think I should. It makes dealing with child nodes in the address bar a lot easier.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: change the mousedown and click handlers to bubbling phase

Change to match the change made to the toolkit version in bug 317369.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: branch patch

Includes the xpfe handler phase changes from bug 312642.

Comment 14

[[e-mail]]

Comment on attachment 210480 [details] [diff] [review]
change the mousedown and click handlers to bubbling phase

r+sr+b+whatever ;-)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 210480 [details] [diff] [review]
change the mousedown and click handlers to bubbling phase

Checked in on the trunk.
mozilla/xpfe/global/resources/content/bindings/textbox.xml; 1.36;

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Created an attachment (id=210491) [edit]
> branch patch

xpfe/toolkit are now synced on the trunk, this mirrors that to the branch (I'll fix the addition of the fields in the toolkit version to group them with the other field when checking in).

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Oops, I accidentally landed the toolkit part already. :(

I fixed some whitespace nits and corrected the log to show the right approval, though, and landed the xpfe version.

mozilla/toolkit/content/widgets/textbox.xml; 1.21.4.3;
mozilla/xpfe/global/resources/content/bindings/textbox.xml; 1.32.4.2;

Comment 18

[:Gavin Sharp [email: [e-mail]]]

VERIFIED fixed using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060822 Minefield/3.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060822 BonEcho/2.0b2