GNOME Bugzilla – Bug 149788
Flat review of "Caret Browsing" dialog in Mozilla incorrectly ellides "OK" and "Cancel"
Last modified: 2004-12-22 21:47:04 UTC
1. Launch Gnopernicus 2. Launch Mozilla with accessibility support 3. Press F7 to turn on caret browsing mode (and to bring up the Caret Browsing dialog) (note that Gnopernicus fails to automatically read this dialog; mozilla bug #254995) 4. Press NumPad-<del> and then start reading the dialog via flat review (NumPad-2 several times) 5. Note that the line of the buttons "OK" and "Cancel" is read as the single word "Okcancel".
Gnopernicus' flat review has some heuristics to provide spaces before the first information in a line, between informations, and after the last one (depends on what flags are set). The problem is that this logic does not work if we do not have at least one object exposing a text interface in the current dialog. In case of "Caret Browsing", there is a 'text' object here but it is not SHOWING so it is not read by gnopernicus. In this situation, gnopernicus can not set some values in order to have spaces before/between/after informations. Basically, it is the same behavior like not using 'Embedded' horizontal flag.
Created attachment 31093 [details] [review] proposed patch I believe the simpliest way to fix this is to make the same thing like for braille or brlmon: put a <space> between chunks befor send the line to speech.
The previous patch does not fix the problem with leading/embedded/trailing flags for objects without Text interface (I agree that this should be fixed inside the flat review algorithm). With or without this patch, the appropriate spaces before/between/after info will not be set. But I believe this patch is necessary even after we have the correct fix in flat review algorithm (it doesn't fix only this particular situation): try to use flat review in Gnopernicus/Preferences/Speech window WITHOUTH any flags - the same concatenation occurs (i.e. "HelpApplyCancelOK" is sent to speech...). But in brlmon the info chunks are separated by a vertical bar (i.e. "|Help||Apply||Cancel||OK|" is sent to brlmon). This delimiter should not be ignored for speech, but converted into a <space> (even we use flags or not). I think this is the best way/place to solve this problem, because if we don't use flags only the strings will be sent for presentation, without any spaces.
Created attachment 31243 [details] [review] proposed patch (in addition to Dana's patch)
Remus: I think it would be better to use g_utf8_strlen in both places in your patch (id=31243) since otherwise, a divide-by-zero is possible.
Created attachment 31576 [details] [review] In addition to Dana's patch, instead of Remus' patch, at Bill's suggestion Please note that without Dana's patch and with all horizontal flags unchecked, the buttons _aren't_ reported correct, the 'concatenation' is present.
I am concerned about the fact that pixels_per_column may be G_MAXINT, since the only place it gets set is in line 845 of (unpatched) screen-review.c. Although it may be ok for this value to be G_MAXINT because it's mostly a denominator (and would thus make the values which use it nearly zero), there are places where you use "pixels_per_column + 1" which clearly will cause trouble if pixels_per_column is G_MAXINT!
Created attachment 31639 [details] [review] a new proposed patch
The new patch contains also the Bill's suggestion to not have G_MAXINT. I think that G_MAXINT + 1 != 0 (because is a sign number) and (xxx / G_MAXINT +1 )== 0 (because of its lower value and integer division), but G_MAXUINT + 1 == 0 (because is a unsign number). Anyway, the ideea was to have a huge number. I think that 10000 is huge enough for sreen-review purposes. And, in the code is xxxx/ G_MAXINT +1 == (xxxx / G_MAXINT) + 1, so, is _never_possible to have troubles because of that.
And, in the code is xxxx/ G_MAXINT +1 != (xxxx / G_MAXINT) + 1, so, is _never_possible to have troubles because of that.
Comment on attachment 31639 [details] [review] a new proposed patch Thanks Remus, I think this patch is OK now. Can you make sure to test it on Solaris?
Bill, I can not test this patch on Solaris because ob bug #152787 (I am using s10_67). Can somebody which has 152787 already fixed test this patch? Thanks!
Patch applied in CVS.
Comment on attachment 31639 [details] [review] a new proposed patch Patch committed in cvs head and gnome-2-8 branch.