GNOME Bugzilla – Bug 527022
updateBraille() has significant whitespace issues, should use braille generators when possible, and fails to underline links
Last modified: 2008-10-07 19:51:30 UTC
We currently do not display linked text as "underlined" in braille -- or give the user the option of enabling this feature. We should.
Can this one by closed off? I believe this functionality will be part of the patch in bug #426010.
OK with me.
We're going to have to handle this specially in Firefox I'm afraid. At least links aren't suddenly being underlined for me. :-) Re-titling the bug to reflect the work that still needs to be done.
Created attachment 111822 [details] [review] Proposed patch This patch adds link braille indicator support for Gecko. I subclassed braille.Component for a specialized Link region that gives the proper attribute mask. Hopefully we could have other scripts support this region too, in oowriter and in default. This gives the benefit of link clicking via the routing keys.
The links are now being underlined correctly with this patch. The main problem I'm seeing is that pressing a touch cursor on a link does not seem to be activating it.
I forgot to comment earlier that we also want the lnk at the end of the links removed.
(In reply to comment #5) > The links are now being underlined correctly with this patch. The main > problem I'm seeing is that pressing a touch cursor on a link does not seem to > be activating it. > Do you mean cursor routing activation is not working? I'm testing this right now, and it seems to be fine. Could you give a specific case when it doesn't? Or is it constant?
Created attachment 112231 [details] [review] Proposed patch This removes the "lnk" indicator at the end of the link. Is this behavior already speced out?
Strangely, I just re-applied this patch to the latest orca trunk and I am now not seeing dots 7 and 8 under the links. In the config UI I do have orca set to show links with dots 7 and 8. This is the case on any web page.
Eitan - any thoughts on this one?
Eitan, I need to deal with the Gecko script's updateBraille for a couple of bugs I'm working on so I'm going to do an initial investigation on this one. Assigning to me for now.
I just tried this patch as is and it is working for me. Mike, mind trying it again?
OK now I think it's just image links that aren't working. Check out the "home link image" at the top of this page.
I'll take a look in a bit, but here's what I'm wondering regardless: When I implement getting the speech and braille contexts at the same time we get the line, much of what's in updateBraille() now is going to get ripped out -- including what's in this patch I suspect -- and the braillegenerators used instead. So if this patch addresses most of the problem, and hence makes Orca better, would it make sense to check it in now? Then we can make sure we underline image links as part of the 'get the contexts when we get the line' bug. Thoughts?
fine with me.
Cool thanks! The sooner I get these little bugs out of the way, the sooner I can tackle that much-needed context refactor. I'll run the regression tests on this patch this evening.
It's not passing the regression tests. :-( Looks like a unicode issue. I'll investigate more tomorrow or Friday.
Created attachment 118135 [details] [review] patch with updated tests This patch updates the regression tests. A couple of the updates were things that just needed updating independent of this patch (like the new Print dialog), but the rest were the result of this patch. 1. We're not displaying the role 'link' any more. That was the bulk. 2. Marked some to-do "BUG?"s: - Before we removed the role link, we used white space to separate the role from the rest of the text. So given the sentence: "Is Willie Walker, our fearless leader, going to the GNOME Summit in Boston?" where "Willie Walker" and "Boston" are each links, we used to braille "Is Willie Walker Link , our fearless leader, going to the GNOME Summit in Boston Link ?" While a little odd, it would probably be odder still to slap the punctuation right up against the "Link" role. However, now that we're underlining links instead, the "shouldn't slap the punctuation on the role" is no longer an issue and we should be able to punctuate -- and presumably should be punctuating -- things like any other sentence. But we're not doing that with this patch. So the example is now written out as "Is Willie Walker , our fearless leader, going to the GNOME Summit in Boston ?" Not ideal, IMHO, but I'm not convinced it's a show-stopper either. Anyhoo.... It would take too long to mark all of the instances where this occurs in the tests, but I did mark a couple to be sure it stays on our radar. We can always do them as part of the updateBraille() refactor. - This patch doesn't address the underlining of images which are also links; just textual links. We should (I assume) be underlining them. I wasn't sure if we should leave the role 'Link' in place until we implement that, or take out the role and worry about implementing the underlining later. Currently we have the role still in place. Again, I didn't mark each instance; only a couple. - We have one instance where there is a link followed by a combo box and the test includes tabbing to the combo box. Before what is visible is the combo box that just gained focus (cursor == 1); now the line is visible (and cursor == 21): - BRAILLE LINE: 'Severity Link : Severity normal Combo' ? ----- + BRAILLE LINE: 'Severity : Severity normal Combo' - VISIBLE: 'normal Combo', cursor=1 + VISIBLE: 'Severity : Severity normal Combo', cursor=21 ? ++++++++++++++++++++ + I *believe* I've solved all of the stalling issues. Yea! If not, I'll catch them next go 'round. I've got one outright "we should fix this now" issue. Check this out: BRAILLE LINE: 'New bug · Browse · Search · Reports · Account · Admin · Help Logged In joanmarie.diggs@gmail.com | Log Out' - VISIBLE: 'New bug · Browse · Search ·', cursor=1 ? ^^ ^^ ^^^ + VISIBLE: 'New bug · Browse · Search �, cursor=1 ? ^ ^ ^ Looks like a unicode character got split in half. :-( I'll try to get to the bottom of that. HOWEVER, of more concern is what happened when I tried to reproduce the issue via actual use. Try this: Navigate to Bugzilla and Tab amongst the links at the top of the page. (New bug, browse, search, etc.) I don't have my actual display hooked up at the moment, but according to the braille monitor, the underlining is off/displaced a bit to the right -- and it becomes further and further displaced to the right as you tab from left to right on that line. :-( My gut is suggesting that this might all be the same issue (i.e. are we treating those 'middle dots' and/or any non-breaking space chars that might be present as two characters when in reality we should be treating them as one??) Like I said, I'll investigate this last issue next. Will, what do you think? Solve the last issue, check in the patch, and leave the other issues as to-do's?
Created attachment 118140 [details] test case Still debugging but here's what I know so far.... 1. Compare tabbing amongst the first and second sets of links in the attached test case. In the first set, the underlining gets progressively more and more off. :-( In the second set, things work as expected. :-) The difference? The presence of multi-byte characters *surrounding* the links in the first case. The issue isn't the new link-related code; the link-related code just brought it to our attention. I could be wrong, but I suspect we'll see something similar once we add support for attributes in Gecko. 2. in the __init__ of the Region class in braille.py, we have the following line: self.rawLine = string.encode("UTF-8") Get rid of that and set self.rawLine to string (unicode), and suddenly the bug I mentioned magically vanishes. Now to figure out exactly what this rawLine is for. We're undoubtedly doing what we're doing for a reason.... :-) Eitan if you're reading this and feel like chiming in with a hint, I'm all ears. ;-)
Okay, I have no idea if this is the right thing to do or not, so feel free to point and laugh (as long as when you're finished, you give me the answer. ;-) ). HOWEVER: If, in the Text class' getAttributeMask(), we base the stringLength on the what we'll be displaying, i.e.: - stringLength = len(self.rawLine) - len(self.label) + stringLength = len(self.rawLine.decode("UTF-8")) - len(self.label) we suddenly seem to do the right thing -- in my extremely brief and limited testing. My gut tells me that this seems safe because we're just changing a single figure in the method for creating the mask, and logical in light of the bug in question. I'll run the regression tests and see if it makes a difference. And then we'll go from there I suppose. :-)
Oops, that blows something up in OOo. :-(
Correction, we blow up just fine without that change. See bug 551077. Back to what I said in Comment #21 - Take 2.
(In reply to comment #21) > My gut tells me that this seems safe because we're just changing a single > figure in the method for creating the mask, and logical in light of the bug in > question. I'll run the regression tests and see if it makes a difference. And > then we'll go from there I suppose. :-) Yikes! There might incorrect assumptions going on in braille.py that we are dealing with strings that do not contain multibyte characters. In general, we should be doing all string length and character analysis using unicode characters, and then converting to UTF-8 for passing to 'external' things. So, your change looks fine. I'm scared about other places where we might be doing the wrong thing in braille.py. But, I guess we can try to table those fears for now and work on getting this stuff to work and not crash when dealing with single-byte characters.
(In reply to comment #19) > should be able to punctuate -- and presumably should be punctuating -- things > like any other sentence. But we're not doing that with this patch. Sounds fair, though definitely not ideal. :-( I'd guess it would be one of the first things users mention. > - This patch doesn't address the underlining of images which are also links; > just textual links. We should (I assume) be underlining them. I wasn't sure if > we should leave the role 'Link' in place until we implement that, or take out > the role and worry about implementing the underlining later. If possible, I think it would be nice to give the user some hint that the thing is a link, so leaving the role 'Link' in place seems reasonable to me. > - We have one instance where there is a link followed by a combo box and the > test includes tabbing to the combo box. Before what is visible is the combo box > that just gained focus (cursor == 1); now the line is visible (and cursor == > 21): Assuming this is being displayed when the combobox has focus, this *seems* like a regression, but I think it's OK for now and just make sure it's marked as BUG? in the tests. > Like I said, I'll investigate this last issue next. Will, what do you think? > Solve the last issue, check in the patch, and leave the other issues as > to-do's? This sounds fair, but just make sure the open issues are marked as BUG? (which I'm sure you'd already do). If possible, though, it would be nice to try to fix the open issues first. Thanks for your work!
Created attachment 118161 [details] [review] Patch to hopefully address the unicode issues Well...my fears wouldn't let me rest. This patch attempts to address the unicode issues in braille.py as well as bug #551077. Not pylinted or regression tested, but it seems to work OK in the hand testing I did with OOo and FF.
Well, bless your fears then. :-) It pylints, it regression tests (OOo, gtk-demo, FF), it fixes bug 541762, it fixes both of the issues at hand -- including half of the multibyte character appearing in one of the tests. Yea! Thanks so much!!! The one change I noticed in some of the FF regression tests is the introduction of a space here and some extra (but proper) characters there. We undoubtedly were dealing with (hacking around) the underlying bug in the Gecko script. So.... I'd like to spend some time today seeing if I can address the space-after-link issue and figure out what our work-around/hack was and if it can be removed. After that, however, I say we check this puppy in and move on. :-) Thoughts?
Unicode patch committed. Thanks for testing, Joanie! > So.... I'd like to spend some time today seeing if I can address the > space-after-link issue and figure out what our work-around/hack was and if it > can be removed. Sounds good to me. Thanks!
Given the complex nature of the problem, implementing this change is going to be too risky for 2.24. Pushing out.
Created attachment 119629 [details] [review] work in progress, here for safe keeping (i.e. don't look) :-) 1. updateBraille() currently takes out valid (i.e. navigable) whitespace (sometimes more than one character's worth) and replaces it with a braille region containing a single space character. This makes a mess of things on several fronts, the most notable w.r.t. this bug being the ability to underline links in braille while preserving the original/desired spacing and punctuation of the document. Still working on this, and it's very much in the "just get it working" phase. 2. Links are underlined -- and should not be surrounded by extra whitespace now. 3. Image links are now underlined and I believe the whitespace with them is also correct. 4. Both are done via the braille generators rather than directly in updateBraille(). Other stuff is as well. 5. Beaucoups of updated regression tests (including little notes to myself, and BUG?'s I still need to work out). I'm attaching this now rather than later out of a paranoid fear that if I don't, I'll somehow accidentally delete it. That said, I think we're pretty close to having this one in the bag.
Created attachment 119656 [details] [review] just about there I think.... (alright, you can look) :-) This version fixes some of the things I needed to work out. There are still a small number of BUG?'s, but they seem to be few and far between. I also tested this some this morning with my Alva Braille Terminal 340 (yes, I'm THAT old). Cursor routing seems to work, as does clicking on/activating links and image links via cursor-routing keys. Yea! I have not yet (re)installed liblouis to see how contracted braille is (or isn't working), but given the nature of the change I would think this patch didn't break anything.... Going to mark this as testing required, because I suspect/hope it might be "good enough for government work" in its current form. There are other open braille-related bugs (displaying headers in table cells, indicating attributes, indicating selection, etc.) in which I can continue my updateBraille() changes/cleanup. :-) I'm also going to repurpose this bug. The bulk of the (code-related) changes in this patch are about all of the whitespace issues plus the need to rely upon the braille generators in updateBraille() more than we do. And, oh, by the way, links are underlined. ;-)
This seems to be working well. One question: Should we also be underlining other clickable items such as buttons? Take for example: the search button on www.google.com. My guess is that we probably should.
(In reply to comment #32) > This seems to be working well. Yea! Thanks Mike!! > One question: Should we also be underlining other clickable items such as > buttons? Take for example: the search button on www.google.com. My guess is > that we probably should. Maybe. I dunno.... 1. Do we underline other clickable things outside of web content? Or is it assumed that the user will see the role and know that buttons are clickable? Whatever we do, I think we should be consistent. 2. That sounds like we might want a "clickable" class in braille.py(??) or add clickable as a property of component and/or region(??) 3. Would we remove the role in that case? 4. What exactly is clickable? I can click on a radio button to select it. I can click on a list to give it focus. I suppose those aren't actions, though. so does "clickable" really mean "actionable"? And if so, combo boxes have associated at-spi actions for expanding and collapsing them -- also accomplished through clicking. Do we underline combo boxes as well -- or only do so when they aren't focused? And if focus is a menu item within the combo box, the menu item does not (I believe) have an action. Would we underline it because the combo box (parent of the parent of the menu item) does? Put another way, I totally defer to your judgment on what we should do around these issues. But these issues sound like they far exceed the scope of the (non-trivial) issues addressed by this bug (whitespace, links, and image links). Therefore, I think what might be worth doing is giving this some more thought and if the decision is indeed to underline clickable objects, open a new bug and spec out the exact behavior and where all it applies. Thoughts?
After more testing I think this patch is worth checking in. I'll file another bug on the new functionality.
Thanks Mike. Done. Before doing a final commit, I always do a number of functional tests just to be darned sure. In doing so I noticed that with this one particular test case using OOo 3.0 beta m7 we were not underlining the links. But then I tested without the patch and we still were not underlining links in that document. Not surprising I guess: This patch is almost exclusively changes in and/or used only by the Gecko toolkit. Still I wanted to be sure. Anyhoo, I'll investigate this OOo issue separately and file a new bug if need be. Wiping brow and moving to pending.
Closing as FIXED. And there was much rejoicing. Yea! :-)