GNOME Bugzilla – Bug 515804
Whitespace needs to be removed from speech and braille contexts in FF3
Last modified: 2008-03-06 19:21:51 UTC
A long, long time ago (okay a few months ago) FF3 was missing whitespace where it rightfully belonged, namely around and between embedded object characters. Thus: <p>This is a <a href="foo">nifty</a> <a href="bar">test</a> indeed.</p> Would be exposed as follows for the accessible text of an object of ROLE_PARAGRAPH: This is aEOCEOCindeed. The lack of spaces where they belong is problematic for, among other reasons, caret navigation (you can arrow to those missing spaces) and text selection (you can select 'em too). However, their introduction has been problematic for us because we all of a sudden have all of this extra whitespace. I attempted to strip some of it out as part of the revised line navigation. But, there's still extra whitespace present in both the speech and braille contexts. In addition, I discovered last night that many of the problems I've been having with the implementation of a reliable (while still performant) line navigation have been the result of removing and/or ignoring whitespace while building up the list of objects on the line. The whitespace needs to stay put. So.... We're going to have to remove it from the speech and braille contexts instead. Shouldn't be too hard. <fingers crossed>
Targeting it for sooner rather than later because I plan to post a new patch for line navigation which will hopefully be far more reliable and still performant -- but will come at the cost of some additional whitespace. :-)
Created attachment 105843 [details] [review] part 1 - strip it out of getDisplayedText() I wound up getting some of the whitespace as part of the recent line nav changes. In looking over the regression test output, one place where we still have lots of whitespace is with the dojo and ARIA widgets, both in the speech and in the braille. The reason why is that with dojo and ARIA, we let the default generators kick in and those default generators call getDisplayedText(). Rather than make a change in default.py, this patch just adds a getDisplayedText() to Gecko.py which simply calls default.py's and then strips out the whitespace. In running the regression tests, I am not seeing any regressions; I am seeing extra whitespace being removed from the following: dojo_dialog.py dojo_slider.py moz_checkbox.py moz_menu.py moz_tabpanel.py Scott, I know there are other dojo and ARIA issues (e.g. double braille), which I will look at next. In the meantime, could you please let me know if the change from this patch solves the whitespace issues you mentioned? Thanks!
Created attachment 106040 [details] [review] revision 2 - don't remove it from entries During team meeting today, Will raised the valid concern about the potential for stripping out whitespace from entries when we shouldn't be doing so. This version doesn't strip whitespace if the object for which we're calling getDisplayedText() is of ROLE_ENTRY. It pylints, it regression tests, it removes extra whitespace in: * moz_checkbox.py * moz_menu.py * moz_tabpanel.py * dojo_dialog.py Mike please test. Scott, please let me know what you think. Will, does this strike you as being safe and the right thing to do?
(In reply to comment #3) > Created an attachment (id=106040) [edit] > revision 2 - don't remove it from entries > > During team meeting today, Will raised the valid concern about the potential > for stripping out whitespace from entries when we shouldn't be doing so. This > version doesn't strip whitespace if the object for which we're calling > getDisplayedText() is of ROLE_ENTRY. Should we consider doing this for any text area that is STATE_EDITABLE?
> Should we consider doing this for any text area that is STATE_EDITABLE? Yup, you're right. And we should add ROLE_PASSWORD_TEXT into the mix as well. i.e. don't strip it if it's * an entry or a password field regardless of whether or not it's editable * if it's some other beast regardless of role which happens to be editable If we do that I think we'll be maximizing our odds of correctly identifying the whitespace we want to strip. After the pylinting and regression testing is done (assuming it pans out), I'll post a new patch. Thanks!
The latest patch does indeed help with the whitespace issue for many of the ARIA regression tests, including the Mozilla checkbox, menu and tabpanel tests. However, the label on the Dojo slider page ("Slider 1 Value: ") is completely missing. The labeling technique the Dojo guys used here is really a big hack. The label is a substring of a larger string residing in the 'form' which includes the labels above the slider and line breaks for positioning. I'm not so sure if we want to jump though hoops to support this. Also of note is that this is an HTML issue and is not ARIA related. I vote for including this patch.
> However, the label on the Dojo slider page ("Slider 1 Value: ") is completely > missing. I'm running the regression tests on the new version now so I can't look at earlier output. However, my recollection is that the label was already missing in the speech (but was present in the braille). Did that change as a result of this patch (i.e. with this patch is it also missing from the braille now?) If it's not related to this patch.... > so sure if we want to jump though hoops to support this. Also of note is that > this is an HTML issue and is not ARIA related. I have some work to do w.r.t. our label guessing strategies and was already planning on looking at the test you mentioned as part of that. If the problem you mention is related to this patch, however, I want to take a closer look before committing. Thanks!!
(In reply to comment #7) > > However, the label on the Dojo slider page ("Slider 1 Value: ") is completely > > missing. > > I'm running the regression tests on the new version now so I can't look at > earlier output. However, my recollection is that the label was already missing > in the speech (but was present in the braille). Did that change as a result of > this patch (i.e. with this patch is it also missing from the braille now?) > Apparently it is not due to this patch. I am seeing no labeling for Braille or speech with and without the patch.
Created attachment 106104 [details] [review] revison 3 - or password text or anything with STATE_EDITABLE Doesn't seem to impact the current regression tests; should hopefully address the issues raised by Will. (Note to self: Add tests that reflect the issues raised by Will). Will how's this version grab ya? Thanks! (And Scott, I'll investigate the label issue you raised).
(In reply to comment #9) > Will how's this version grab ya? Thanks! This looks fine, though I'm surprised that the EDITABLE state does not already exist on password or entry fields. That is, is the role check actually redundant?
You can have read-only entries and password fields I believe (protected via javascript). And if those contain spaces or newlines, I don't think we want to remove those characters from the context. Thus I don't *think* it's redundant, nor do I think it's an expensive check. By the same token, if I had a nickel for every time I was wrong about something, I wouldn't need my DayJob and could work on Orca full time. ;-) So I'm happy to take the role check out if you think that makes sense.
(In reply to comment #11) > You can have read-only entries and password fields I believe (protected via > javascript). Ahh...got it. OK. Just checking. Patch looks fine to me, then. I say commit to trunk and gnome-2-22 if it passes the regression testing and all that jazz. I'm still not sure I understand why the space is there in the first place, though. From what I can tell, it looks like FF is giving us the text to us this way. Is this a FF bug? Is this something where the content provider has purposely embedded whitespace in the content? Are we sure that stripping in these cases is really the right thing to do?
Created attachment 106108 [details] an entry (with extra whitespace) that lacks STATE_EDITABLE Turns out you don't even need the shiny miracle that is javascript. :-)
In terms of this patch, I believe it's either Firefox bugs or Firefox features depending on one's perspective. Scott has said something about role:presentation as I recall, but I can't find his message (or the bug) atm. When providing access to HTML content (as opposed to dojo and ARIA), we really aren't using getDisplayedText() all that much so this patch has limited impact on that front. As for do it or don't do it: Given something like... ------------ ----------- | | | | | BTN1 | | BTN2 | | | | | ------------ ----------- (yes, kids, I did get a 'D' in 7th grade art) and an 18-cell braille display (e.g. a user hooking up their braille notetaker to use as a display), what should we do if the user arrows down to the line with the two buttons? I for one think all that space in between the buttons is unnecessary as well as *normally* undesirable. It's somewhat hard to compare what we do in terms of the presentation of web content with what we do when presenting other information, but here's the closest example I can think of: Get into the Orca Preferences dialog, move focus to the Help button, and use flat review (KP8) to review the current line. There's a heck of a lot of empty real estate between the Help button and the Apply button, but we don't seem to preserve it: SPEECH OUTPUT: 'Help Apply Cancel OK' BRAILLE LINE: 'Help Apply Cancel OK $l' VISIBLE: 'Help Apply Cancel OK $l', cursor=1 Should we be preserving it there?
Oh, I forgot to mention, this patch seems to preserve non-breaking space characters. Thus if the web page designer purposefully went in and added extra space via , we're leaving it in there.
I feel a little uncomfortable with the possibility that we might be stripping things out that are intentionally there, but I'm OK with the checks that are in place. I say commit if it passes muster.
I've given this quite a bit of testing and haven't noticed any problems with to much space being removed.
Patch committed to both the gnome-2-22 branch and to trunk. Moving to pending.
Updating the target to reflect when the patch was committed.