Opened 11 years ago

Closed 10 years ago

Last modified 8 years ago

#8878 closed enhancement (fixed)

[patch][cla] dijit.form.ComboBox would like ability to extend announce label

Reported by: Nathan Toone Owned by: Nathan Toone
Priority: high Milestone: 1.4
Component: Dijit - Form Version: 1.3.0b3
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Nathan Toone)

Sometimes, it is desirable to have a different announce label than the search terms. However, it is difficult to override this behavior in a subclass.

I suggest breaking out a protected function that will return the value to display for "announcements"

Attachments (1)

comboBoxAnnounce.patch (624 bytes) - added by Nathan Toone 11 years ago.
Read from labelAttr, if it exists. Fall back to searchAttr. This is what we do in other places. This can be committed post 1.3…

Download all attachments as: .zip

Change History (15)

Changed 11 years ago by Nathan Toone

Attachment: comboBoxAnnounce.patch added

Read from labelAttr, if it exists. Fall back to searchAttr. This is what we do in other places. This can be committed post 1.3...

comment:1 Changed 11 years ago by Douglas Hays

Milestone: tbd1.4
Owner: set to Nathan Toone

comment:2 Changed 11 years ago by Nathan Toone

Milestone: 1.41.3.1

Fixed in trunk in [17155] - changing milestone for consideration for inclusion in branch.

comment:3 Changed 11 years ago by Nathan Toone

Milestone: 1.3.11.4
Resolution: fixed
Status: newclosed

comment:4 Changed 11 years ago by haysmark

Priority: lowhigh
Resolution: fixed
severity: minormajor
Status: closedreopened

toonetown, your change introduces a curious regression:

  1. Go to http://archive.dojotoolkit.org/nightly/checkout/dijit/tests/form/_autoComplete.html?testWidget=dijit.form.ComboBox
  2. Scroll down to "Custom labelFunc", which uses HTML labels
  3. Clear the input
  4. Arrow down to Alabama
  5. Observe the contents of the ComboBox? and listen to what the screen reader says

You will find with your change, that the ComboBox? now contains the following text:

<img width='97px' height='127px' src='images/Alabama.jpg'/>Alabama

This is the text for an image tag! Also, the screen reader makes no attempt to parse that markup, so it announces the markup itself: "i m g, width equals 97 p x, height equals ..."

You might make it so the ComboBox? only announces the label when it is plain text, but I'm concerned that's inconsistent with the HTML case. What do you think?

comment:5 Changed 11 years ago by Nathan Toone

That's a very good point. What about adding in a different option - "announceAttr", and default it to the same value as searchAttr?

That gives the option for announcing something separate from the search or label attr...

In either case, I have rolled this patch back for now until we decide what to do - rolled back in [17187]

comment:6 Changed 11 years ago by bill

Hmm, I have a hard time imagining dijit users taking the time to learn what an announceAttr is. Could we just do innerText/textContent to get the plain text?

comment:7 Changed 11 years ago by haysmark

The potential problem with innerText/textContent is that it might create unexpected behavior. For instance:

Before<img width='97px' height='127px' src='images/Alabama.jpg'/>Alabama

The text content is "BeforeAlabama?" with no space in between. Yes it could be avoided, but it was unexpected considering a screen reader could parse that just fine.

Here's a thought: after we released 0.9, we got rid of a 'keyAttr' in FilteringSelect? for identifying the primary key of the data. How did we do this? We put it in the store as its "identifier":

{
  identifier:"abbreviation",
  label: "name",

(just noticed there is also a "label" in there, shouldn't we get rid of labelAttr too?) So we could similarly add an optional "alt" or "title" attribute to the incoming data.

Unrelated: in the previous patch, the code was setting the newValue variable to the label text. That could be a problem since that same value gets passed to the autocomplete logic, where ComboBox? expects the searchAttr text in the box when it is autocompleting.

comment:8 Changed 11 years ago by bill

Ah sorry, I misunderstood what was going on. The screen reader discussion seems irrelevant, as the mere fact that the textbox gets the value

Before<img width='97px' height='127px' src='images/Alabama.jpg'/>Alabama

shows a problem.

As Mark said above, the value that gets put into the textbox is subsequently used by the autocomplete logic, being matched against searchAttr, so it seems to make sense to keep them the same.

Nathan, hy is it that you want the textbox to get a value different than the searchAttr? Was it something like capitalization? It seems like this might better be a job for DropDownSelect?

comment:9 Changed 11 years ago by Nathan Toone

Resolution: wontfix
Status: reopenedclosed

I am actually using FilteringSelect? - because I want it to be be filterable. But the announce part of it comes from ComboBox?. The problem happens when you have a labelAttr that is different than your searchAttr (which is completely valid).

However, this is completely not worth any of this discussion time...I'm fine with closing this bug and just overriding this function in my subclass. I had just (originally, and erroneously) thought that it was an oversight (because everywhere else we put values into the textbox portion of the combobox, we use labelAttr
searchAttr.)

comment:10 Changed 10 years ago by Nathan Toone

Description: modified (diff)
Priority: highnormal
Resolution: wontfix
Status: closedreopened
Summary: [patch][cla] dijit.form.ComboBox announce option not honoring labelAttr, if set[patch][cla] dijit.form.ComboBox would like ability to extend announce label

I would like to reopen this bug - and make it a bit easier to extend this functionality in a subclass.

comment:11 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: reopenedclosed

(In [17654]) Fixes #8878 - break out the announce string into a separate (overridable) function

comment:12 Changed 10 years ago by Nathan Toone

Type: defectenhancement

comment:13 Changed 10 years ago by Nathan Toone

severity: majorminor

comment:14 Changed 8 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.