Closed Bug 560270 Opened 14 years ago Closed 14 years ago

Deal with bug 559710 adding another child to the text node in test_combobox.xul

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED WORKSFORME

People

(Reporter: ehsan.akhgari, Assigned: davidb)

References

Details

(Whiteboard: aa) 

With the fix in bug 559710, accessible/tree/test_combobox.xul now requires two children for autocomplete2.  I'm filing this bug so that someone can try to figure out what's going on here.

I'm tentatively assigning this to David, as he vaguely assumed the responsibility of doing that in our conversation!  :-)
Assignee: bolterbugz → ehsan
So, this text node is coming from the textnode under the value div.  There would be a problem with just ignoring this textnode.  When the value is empty, we convert it to "\n" for line-height calculations, and the a11y code converts that newline into a space and exposes the text leaf node with that space.  This means that a11y clients will think that there's a space inside the text box, while in fact there isn't any space there.

I think the correct way to fix it is to look at the content node (or maybe the frame) for retrieving the value, instead of going through the textnode contents.

OTOH, we believe that accessibility clients do not use the text leaf contents to get the value of the textbox.  David is going to confirm this with one of his contacts, and we'll wait for him to get back at us.
(In reply to comment #1)
> OTOH, we believe that accessibility clients do not use the text leaf contents
> to get the value of the textbox.  David is going to confirm this with one of
> his contacts, and we'll wait for him to get back at us.

At least one popular screen reader uses IAccessibleText, which does expose the space. Need to fix this.
Summary: Figure out why the fix to bug 559710 adds another child to the text node in test_combobox.xul → Deal with bug 559710 adding another child to the text node in test_combobox.xul
I forgot to mention that we discovered the space is visible via caret browsing. It seems wrong that there is a space that is not reflected via GetValue.

Should caret browsing into the text control cause the editor to instantiate itself?

Should nsIAccessibleText usage cause the editor to instantiate itself?
(In reply to comment #3)
> I forgot to mention that we discovered the space is visible via caret browsing.
> It seems wrong that there is a space that is not reflected via GetValue.

Does caret browsing use the accessibility API?  If not, this should be fixed separately.

> Should caret browsing into the text control cause the editor to instantiate
> itself?

This is a question that maybe Neil can answer (or knows who needs to answer this; I'm CCing bz and roc as well).  My expectation as a user is that if you put the caret using the caret browsing feature into a text box, you should be able to type in it, but that doesn't seem to be the case even in Firefox 3.6.

> Should nsIAccessibleText usage cause the editor to instantiate itself?

What does nsIAccessibleText do exactly?  I may be able to answer this question if I know that.
(In reply to comment #4)
> (In reply to comment #3)

> > Should nsIAccessibleText usage cause the editor to instantiate itself?
> 
> What does nsIAccessibleText do exactly?  I may be able to answer this question
> if I know that.

It does pretty much everything except boil eggs.

Related platform API specs are here:
http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible_text.html
http://library.gnome.org/devel/atk/unstable/AtkText.html

Our implementation can be seen in:
/accessible/src/html/nsHyperTextAccessible
> Does caret browsing use the accessibility API?

No.

Interaction of caret browsing and editors... <sigh>.  I have no idea how they're supposed to interact.  :(
(In reply to comment #4)
> > Should caret browsing into the text control cause the editor to instantiate
> > itself?
> 
> This is a question that maybe Neil can answer (or knows who needs to answer
> this; I'm CCing bz and roc as well).  My expectation as a user is that if you
> put the caret using the caret browsing feature into a text box, you should be
> able to type in it, but that doesn't seem to be the case even in Firefox 3.6.

Seems like nsFocusManager::GetFocusInSelection should be handling other types of focusable elements. Currently, it only handles links. That is, moving the caret over a link focuses it.

That is, assuming that moving the caret should traverse into the <input>.
Whiteboard: aa
(In reply to comment #6)
> > Does caret browsing use the accessibility API?
> 
> No.
> 
> Interaction of caret browsing and editors... <sigh>.  I have no idea how
> they're supposed to interact.  :(

Yeah very under-specified. Maybe we can trail-blaze.

Jamie I seem to recall NVDA really wants focus and caret synced correct? (http://bit.ly/b5yzyA)

Can anyone think of reasons that keyboard focus should not follow the browse caret?
(In reply to comment #8)
> (In reply to comment #6)

> Can anyone think of reasons that keyboard focus should not follow the browse
> caret?

Discussing with Ehsan and Neil. We'd have to make sure that the user could caret browse out of the control, currently setting focus to the control, traps us.
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> 
> > Can anyone think of reasons that keyboard focus should not follow the browse
> > caret?
> 
> Discussing with Ehsan and Neil. We'd have to make sure that the user could
> caret browse out of the control, currently setting focus to the control, traps
> us.

David, is this something that would fall into the scope of the keyboard behavior document you guys have been working on?
Ehsan, yes I think this should be covered there. I was waiting until Alexander wakes up to chime in on that.
For this bug I think we can put an attribute on the div like nsAccessibilityAtoms::mozeditorbogustree (was mozeditorbogusnode). Then we'll look for this in nsAccUtils::MustPrune. I can do that tonight or tomorrow I think. Taking.
Assignee: ehsan → bolterbugz
(Note to self: also consider nsHyperTextAccessible::DOMPointToHypertextOffset)
It would be better to avoid having to add a magic attribute, that has its own performance cost.

The DIV has the "anonymous-div" class, maybe we could make that more unique and you could look for that. Or maybe you could just check the type of the parent frame.
Depends on: 560665
Depends on: 560669
No longer depends on: 560665, 560669
(In reply to comment #8)
> Jamie I seem to recall NVDA really wants focus and caret synced correct?
> (http://bit.ly/b5yzyA)
It'd be nice, but:

> Can anyone think of reasons that keyboard focus should not follow the browse
> caret?
IN the thread you linked above, Aaron and others seemed to be arguing that this would definitely be a bad thing.
(In reply to comment #10)

> David, is this something that would fall into the scope of the keyboard
> behavior document you guys have been working on?

(In reply to comment #11)
> Ehsan, yes I think this should be covered there. I was waiting until Alexander
> wakes up to chime in on that.

Sorry I was inactive on this last time. I'll send the proposal to the discussion this week.
(In reply to comment #14)
> It would be better to avoid having to add a magic attribute, that has its own
> performance cost.

Yeah, not a big fan of kludges either :)

> The DIV has the "anonymous-div" class, maybe we could make that more unique and
> you could look for that. Or maybe you could just check the type of the parent
> frame.

Thanks Roc we'll probably have to do something like that. What if GetChildren (or nsTextControlFrame::AppendAnonymousContentTo) could bail if the call is from accessibility and the editor is not initialized? Not sure of the best way to do that.

(In reply to comment #15)
> (In reply to comment #8)
> > Jamie I seem to recall NVDA really wants focus and caret synced correct?
> > (http://bit.ly/b5yzyA)
> It'd be nice, but:
> 
> > Can anyone think of reasons that keyboard focus should not follow the browse
> > caret?
> IN the thread you linked above, Aaron and others seemed to be arguing that this
> would definitely be a bad thing.

Yeah I think the right thing to do is to make sure whatever the caret is on is initialized, but additionally giving focus is probably too dangerous.
> (In reply to comment #15)
> > (In reply to comment #8)
> > > Jamie I seem to recall NVDA really wants focus and caret synced correct?
> > > (http://bit.ly/b5yzyA)
> > It'd be nice, but:
> > 
> > > Can anyone think of reasons that keyboard focus should not follow the browse
> > > caret?
> > IN the thread you linked above, Aaron and others seemed to be arguing that this
> > would definitely be a bad thing.
> 
> Yeah I think the right thing to do is to make sure whatever the caret is on is
> initialized, but additionally giving focus is probably too dangerous.

As discussed with David, I don't think that we need to initialize the editor if the caret is inside it, but it's not focused.
Right.
No longer depends on: 376369
(In reply to comment #17)
> Thanks Roc we'll probably have to do something like that. What if GetChildren
> (or nsTextControlFrame::AppendAnonymousContentTo) could bail if the call is
> from accessibility and the editor is not initialized? Not sure of the best way
> to do that.

The editor might be initialized, but have a non-empty value. I suppose we could bail if the editor is not initialized and the value is empty... but that could confuse other users that are looking at the anonymous content.

I think you should deal with this in accessibility.
(In reply to comment #20)
> (In reply to comment #17)
> > Thanks Roc we'll probably have to do something like that. What if GetChildren
> > (or nsTextControlFrame::AppendAnonymousContentTo) could bail if the call is
> > from accessibility and the editor is not initialized? Not sure of the best way
> > to do that.
> 
> The editor might be initialized, but have a non-empty value. I suppose we could
> bail if the editor is not initialized and the value is empty... but that could
> confuse other users that are looking at the anonymous content.

Yeah that's why I was thinking it should only happen for a11y (but that makes my spidey sense tingle).

> I think you should deal with this in accessibility.

OK.
I took a deep look at how this is exposed on windows using Accprobe. There is no difference in the MSAA/IA2 tree (including IAccessibleText) with or without Ehsan's patch. Given this, and Marco's test results I am calling your 559710 patch safe to land (along with the proposed change to the a11y test).

We have 560669 for follow up on whether to cache the textleaf internally. If/When this bites gecko based AT users we can spend time there.

I spoke with Ehsan and have agreement that this bug can close.

Thanks everyone.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
(In reply to comment #22)
> I took a deep look at how this is exposed on windows using Accprobe. There is
> no difference in the MSAA/IA2 tree (including IAccessibleText) with or without
> Ehsan's patch. Given this, and Marco's test results I am calling your 559710
> patch safe to land (along with the proposed change to the a11y test).

I thought this bug was about wrong results in test_combobox.xul, not about MSAA.

> We have 560669 for follow up on whether to cache the textleaf internally.
> If/When this bites gecko based AT users we can spend time there.

perhaps this bug is dupe?
Verified in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre (.NET CLR 3.5.30729) that this really doesn't cause any unwanted spaces or the like in NVDA's and other virtual buffers.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.