446550 - xbl widget for a textbox with a clear button

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, enhancement)

Description

[Daniel Brooks [:db48x]]

This has been implemented in several places at one time or another, but hasn't been a part of toolkit. Patch coming.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: patch

This copies from the places implementation, though the places implementation also does other things so I haven't made places use it.

Comment 2

[Kai Liu]

Are there places where we use a clearable textbox?  Please correct me if I'm wrong, but the only places where I've seen a clear button are the search textboxes (bug 388811).

Comment 3

[Daniel Brooks [:db48x]]

Kai: the search textbox in Places and in that patch are essentially a combination of this binding and the existing timed textbox. I need something like this for Fennec (bug 441794), I worked on a non-mozilla project a while back that also implemented this.

Since XBL doesn't have multiple-inheritance however, both bindings are necessary.

Comment 4

[Neil Deakin]

Why do you need a clear button for bug 441794?

If this is a search-as-you-type textboxes, then we should just finish  implementing bug 388811.

Comment 5

[Daniel Brooks [:db48x]]

It's not for searching, it's for allowing the user to input a new shortcut. When the user decides to assign a shortcut to some command (maybe he wants to assign Control-Alt-Shift-Hyper-D to the command that deletes the third most recently added bookmark), he double clicks on that row of the tree and hits the key combination that he wants. This is great, but the normal ways of clearing a textbox are now unavailable here. Backspace is a great shortcut for going back in the history, and Escape is great for dismissing things (such as the bookmarks list or the preferences window.) I'm sure someone can think of something to assign Delete to as well. That just leaves visible buttons on the screen for him to prod.

I could easily make it Fennec-specific, but since I've already written at least one other app that needed it, I'd rather add it to toolkit where it will be more generally available.

Comment 6

[Dão Gottwald [:dao]]

I think this is special and rare enough that we don't want it in toolkit. I'm not even sure I understand the use case... If you want Escape as a shortcut key for the user to assign, why do you still use it to reset the input?

Comment 7

[Daniel Brooks [:db48x]]

(In reply to comment #6)
> I think this is special and rare enough that we don't want it in toolkit. I'm
> not even sure I understand the use case... If you want Escape as a shortcut key
> for the user to assign, why do you still use it to reset the input?
> 

I don't and I can't use escape to reset the input. If the user hits the escape key, the word "Escape" appears in the textbox, and the escape key is assigned to the shortcut he's editing. Same with backspace, delete or any other key or key combination he can type, which is why there needs to be a button for him to click on instead.

Comment 8

[Dão Gottwald [:dao]]

Maybe I'm missing something, but this is from your patch:

+      <handler event="keypress" keycode="VK_ESCAPE"><![CDATA[ 
+        this._resetInternal();
+      ]]></handler>

And not allowing this would be an accessibility problem.

Comment 9

[Daniel Brooks [:db48x]]

Correct. The widget I've created here is generic, and allows the escape key to reset the textbox. The specific use of it that I'm working on right now for bug 441794 cannot allow any key for resetting the textbox. None of the shortcut editing behavior I've described here is included in the patch for this bug; this bug is just about a textbox that has a specific reset semantic and several ways of triggering it, the primary one being a visible button the user can prod. My apologies for any confusion.

Oh, and the patch also includes a tweak to the tree binding so that you can easily use non-vanilla textboxes with editable trees which is a pretty helpful change in and of itself.

Comment 10

[Dão Gottwald [:dao]]

Yeah, so the specific use case that we have couldn't even use that binding as intended but only in a suboptimal way (inaccessible as it seems). Making this generic is a good approach, but that's only useful if there are other use cases. In which way did the other app need this?

Comment 11

[Daniel Brooks [:db48x]]

The other app needed it in the regular, straight forward fashion. In many ways it was a very boring app.

Comment 12

[Dão Gottwald [:dao]]

Straight forward as in search widget?

Comment 13

[Daniel Brooks [:db48x]]

no, by straight forward I mean it used a textbox in exactly the fashion made possible by this patch.

Comment 14

[Neil Deakin]

(In reply to comment #13)
> no, by straight forward I mean it used a textbox in exactly the fashion made
> possible by this patch.

You're getting recursive here. Can you explain why you needed a clear button in the textbox? That is, why the lack of a clear button made some task impossible or difficult?

Comment 15

[Daniel Brooks [:db48x]]

(In reply to comment #14)
> (In reply to comment #13)
> > no, by straight forward I mean it used a textbox in exactly the fashion made
> > possible by this patch.
> 
> You're getting recursive here. Can you explain why you needed a clear button in
> the textbox? That is, why the lack of a clear button made some task impossible
> or difficult?
> 

See comment 5.

Comment 16

[Neil Deakin]

(In reply to comment #15)
> See comment 5.

I meant explain why the other application you mentioned needed this. I want to know whether this has other uses besides that for entering keyboard shortcuts.

Comment 17

[Daniel Brooks [:db48x]]

It wasn't anything special, it was just what the client requested. I've no problem with it being in Fennec only, I just figured it'd be useful.

Comment 18

[Daniel Brooks [:db48x]]

Attachment: 446550-2.diff

Here's an alternate patch that adds the binding to Fennec instead. Either way, I'll still need to get the to tree.xml checked in to toolkit.

Comment 19

[Neil Deakin]

Comment on attachment 340746 [details] [diff] [review]
446550-2.diff

>+          var handler = this.getAttribute("onreset");
>+          if (handler) {
>+            var handlerFunction = new Function("event", handler);
>+            handlerFunction(event);
>+          }

These last five lines aren't needed for the reset event as it's recognized as a known event.

Otherwise, this looks ok.

Comment 20

[Stuart Parmenter]

did this land?

Comment 21

[Stuart Parmenter]

this feels like it makes a lot more sense in toolkit, or some kind of "mobile toolkit," but not directly in Fennec.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 340746 [details] [diff] [review]
446550-2.diff

>+.textbox-input-closebutton {
>+  -moz-appearance: none;
>+  margin: 1px 2px 1px 1px;
>+  min-width: 0;
>+  background-color: transparent;
>+  border: none;
>+  padding: 0 !important;
>+  list-style-image: url("chrome://global/skin/icons/Search-close.png");
>+  -moz-image-region: rect(0px 16px 16px 0px);
>+  width: 14px;
>+  height: 14px;
>+  cursor: default;
>+}
>+
>+.textbox-input-closebutton > .button-box {
>+  margin: 0;
>+  padding: 0;
>+  border: 0;
>+}

Using a xul:image instead of a button would make this significantly easier.

Comment 23

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

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

Daniel was trying an alternative approach and was going to post a new patch with only the tree.xml changes I think? I know we talked about this on IRC, but I don't remember what was said.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

did this land?

Comment 25

[Matt Brubeck (:mbrubeck)]

No, this did not land as far as I (using MXR and "hg grep") can tell.  Does this bug need to be open still?

Comment 26

[BMO Automation]

Closing all opened bug in a graveyard component