392890 - Provide a text label for the Search edit in Download Manager

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P3)

Description

[Marco Zehe (:MarcoZ)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: trunk

In the new Download Manager, the Search... textbox does not have an associated label.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield.
2. Select Tools/Download Manager.
3. Tab to the Search... textbox.
Actual Results:  
No label is spoken for this textbox.

Expected Results:  
Search: or some equivalent label should be spoken.

Comment 1

[Aaron Leventhal]

This is a Firefox-specific dialog, so moving out from Core to Firefox.

Comment 2

[Aaron Leventhal]

Please see the search box to see how we created an accessible label for that.

Comment 3

[Aaron Leventhal]

I should say, please see the web search box in the main browser window.

Comment 4

[Ed Lee :Mardak]

Bug 399783 will remove the search box, so this bug would become invalid.

Comment 5

[Mike Connor [:mconnor]]

P3 if we're keeping this.

Comment 6

[Ed Lee :Mardak]

Attachment: v1

(In reply to comment #2)
> Please see the search box to see how we created an accessible label for that.
The search engine box has a special label getter to support different search engines.. Is adding a label all that's needed for the download manager search?

Comment 7

[Aaron Leventhal]

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

This works but we're better off with a general solution in mozilla/accessible/src/xul/nsXULTextfieldAccessible.cpp for the case where there is a grayed label inside the textbox.

We should override GetName to use the new emptyText when there is no label.

Comment 8

[Aaron Leventhal]

Actually I think this should be fixed in bug 406095. They should probably just expose the emptyText as the label when there is no other label.

Comment 9

[Ed Lee :Mardak]

Actually, didn't realize bug 388811 was blocking this. Seems like Mossop will be able to get a unified search box implementation which should have all the label, grayText, emptyText stuff.

Comment 10

[Shawn Wilsher :sdwilsh]

Status update?

Comment 11

[Ed Lee :Mardak]

Attachment: v2

Use the new emptyText property instead of doing it ourself. I'll r? sdwilsh after the Dão looks at it.. he might want to get rid of that TODO comment and flex and resizer..

Comment 12

[Ed Lee :Mardak]

Comment on attachment 301704 [details] [diff] [review]
v2

>-  if (!gSearchBox.hasAttribute("empty"))
>+  if (gSearchBox.value != "")
Oh, I suppose technically I didn't have to change that.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 301704 [details] [diff] [review]
v2

Hey, I assigned this bug to myself for a reason :)

This patch doesn't work (emptyText property vs. emptytext attribute), and I there's more cleanup to do.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch

Comment 15

[Dão Gottwald [:dao]]

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

>diff -u -8 -p -r1.24 downloads.css
>--- toolkit/themes/pinstripe/mozapps/downloads/downloads.css	29 Jan 2008 05:16:35 -0000	1.24
>+++ toolkit/themes/pinstripe/mozapps/downloads/downloads.css	6 Feb 2008 21:54:18 -0000
>@@ -94,12 +94,8 @@ richlistitem[type="download"] button {
>   -moz-border-right-colors: #969696 #C5C5C5 -moz-Field;
>   -moz-border-left-colors: #969696 #C5C5C5 -moz-Field;
>   -moz-border-radius: 11.5px;
>   background: url("chrome://global/skin/icons/search-textbox.png") -moz-Field no-repeat 1px center;
>   -moz-background-clip: padding !important;
>   padding: 0 8px;
>   -moz-padding-start: 16px;
> }
>-
>-#searchbox[empty] {
>-  color: -moz-Field;
>-}

Btw, I don't really understand this. Seems like this would make the emptyText invisible on Mac (same background and foreground color).

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

I just realized that the regular expression for trimming the search terms isn't quite correct. Let's fix it while we're touching it.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Btw, I don't really understand this. Seems like this would make the emptyText
> invisible on Mac (same background and foreground color).
Yeah, my current nightly doesn't have any text.  I'd been wondering about that for a little while, but no time to investigate it.

Comment 18

[Ed Lee :Mardak]

madhava: Should the download manager search box show the text "Search..." if the user hasn't entered anything?

The new theme hides the text by making it the same as the background color, but in its place is a search icon. So technically it doesn't regress bug 391861.

Comment 19

[Ed Lee :Mardak]

madhava: Oops, forgot to specify that it's OS X only. (I suppose additionally, OS X download manager has the search box on the right side.. should the other platforms conform? [if so, I'll file a separate bug])

Comment 20

[Ed Lee :Mardak]

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

r=Mardak

>   <hbox id="search">
>     <textbox type="timed" timeout="500" id="searchbox"
>-             oncommand="buildDownloadList();" empty="true"
>-             value="&searchBox.label;" defaultValue="&searchBox.label;"
>-             onblur="onSearchboxBlur();" onfocus="onSearchboxFocus();"/>
>+             oncommand="buildDownloadList();" emptytext="&searchBox.label;"/>
>     <spacer flex="1"/>
>-    <!-- TODO get advanced search working (Bug 390491)
>-    <button label="Advanced Search"/>
>-    -->
>     <resizer id="windowResizer" dir="bottomright"/>
>   </hbox>
Could you also kill that spacer and resizer? 

>+++ toolkit/themes/pinstripe/mozapps/downloads/downloads.css	6 Feb 2008 21:54:18 -0000
And remove the "#search > spacer" from this file.

Comment 21

[Ed Lee :Mardak]

Oh, forgot to mention..

<madhava> I think it should have the text

Comment 22

[Dão Gottwald [:dao]]

Attachment: ... and remove the resizer

Comment 23

[Reed Loden [:reed]]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.136; previous revision: 1.135
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.48; previous revision: 1.47
done
Checking in toolkit/themes/gnomestripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/themes/pinstripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/themes/winstripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.28; previous revision: 1.27
done

Comment 24

[Marco Zehe (:MarcoZ)]

Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022005 Minefield/3.0b4pre. Thanks Stephen for the pointer!

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #24)
> Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US;
> rv:1.9b4pre) Gecko/2008022005 Minefield/3.0b4pre. Thanks Stephen for the
> pointer!

Marco - would you mind writing a Litmus testcase for this?  I can write one, but am unsure I'd get the fine-grained details right for the various screen-readers.  Thanks, either way!

Comment 26

[Marcia Knous [:marcia]]

I created https://litmus.mozilla.org/show_test.cgi?id=5305 for Litmus, but I did not enable it - Marco if you can tweak any verbiage necessary and then enable the test case that would be great.

Comment 27

[Marco Zehe (:MarcoZ)]

Thanks Marcia! It's now live with a minor edit to the expected text that the user should hear.

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

But the testcase doesn't say anything about accessibility; end-users running this test will mark it as failed since they don't know it's accessibility-specific, which will create noise and mark it hard to tell if/when it really fails, right?

Comment 29

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381