567084 - getTextAttributes should return invalid arg if the given offset is wrong

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

We shouldn't use HypertextOffsetToDOMPoint because it returns node offset pointing to the last child if offset is bigger. I would change it but it might make sense for other callers. While I'm not sure and testsuite doesn't cover this I would prefer to fix getTextAttributes() locally.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 446484 [details] [diff] [review]
patch

Stopping the usage of HypertextOffsetToDOMPoint makes us fail for 0 offset on empty hyper textboxes. I'm going to put new patch.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #1)
> We shouldn't use HypertextOffsetToDOMPoint because it returns node offset
> pointing to the last child if offset is bigger.

Can you explain this in more detail?

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 446499 [details] [diff] [review]
patch2

r=me with comments:

>+ * Build an object of default text attirbutes expected for the given accessible.

Typo: 2Attirbutes" must be "attributes".

>+      // test our of range offset

Typo: must be "out of range..."

>+	<input id="area14">

You never do anything with this one.

Also in this part and in a couple of other places I noticed tab characters instead of spaces.

Thanks for the cleanup!

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #1)
> > We shouldn't use HypertextOffsetToDOMPoint because it returns node offset
> > pointing to the last child if offset is bigger.
> 
> Can you explain this in more detail?

The problem GetPosAndText() returns start and end accessibles always even if offset is greater than length of hyper text accessible. HypertextOffsetToDOMPoint which converts results from GetPosAndText() into DOM offset. That's not desired behavior.

Comment 7

[alexander :surkov (:asurkov)]

Thanks, Marco, fixed!

Comment 8

[alexander :surkov (:asurkov)]

Ehsan, there is no primary frame for text node inside HTML input when editor is not initialized, any idea why?

Comment 9

[(no longer active)]

(In reply to comment #8)
> Ehsan, there is no primary frame for text node inside HTML input when editor is
> not initialized, any idea why?

Hmm, there should be.  When are you trying to retrieve it?  Maybe the frame constructor hasn't had a chance to construct the frame.  But there _should_ be a text frame for the text node (you can verify this using the layout debugger.)

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (In reply to comment #8)
> > Ehsan, there is no primary frame for text node inside HTML input when editor is
> > not initialized, any idea why?
> 
> Hmm, there should be.  When are you trying to retrieve it? 

I'm trying to do this when mochitest is running.

> Maybe the frame
> constructor hasn't had a chance to construct the frame.  But there _should_ be
> a text frame for the text node (you can verify this using the layout debugger.)

Where can I get it?

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Ehsan, there is no primary frame for text node inside HTML input when editor is
> > > not initialized, any idea why?
> > 
> > Hmm, there should be.  When are you trying to retrieve it? 
> 
> I'm trying to do this when mochitest is running.

No, but when, I meant do you do it after frame construction, or during it.

> > Maybe the frame
> > constructor hasn't had a chance to construct the frame.  But there _should_ be
> > a text frame for the text node (you can verify this using the layout debugger.)
> 
> Where can I get it?

Apply my patch from bug 534694 in your tree, and just build using --enable-debug and --enable-tests.  When you launch Firefox, it's available from the Tools menu.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > Ehsan, there is no primary frame for text node inside HTML input when editor is
> > > > not initialized, any idea why?
> > > 
> > > Hmm, there should be.  When are you trying to retrieve it? 
> > 
> > I'm trying to do this when mochitest is running.
> 
> No, but when, I meant do you do it after frame construction, or during it.

If I would know. How can know this?

> > > Maybe the frame
> > > constructor hasn't had a chance to construct the frame.  But there _should_ be
> > > a text frame for the text node (you can verify this using the layout debugger.)
> > 
> > Where can I get it?
> 
> Apply my patch from bug 534694 in your tree, and just build using
> --enable-debug and --enable-tests.  When you launch Firefox, it's available
> from the Tools menu.

Thanks.

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > Ehsan, there is no primary frame for text node inside HTML input when editor is
> > > > > not initialized, any idea why?
> > > > 
> > > > Hmm, there should be.  When are you trying to retrieve it? 
> > > 
> > > I'm trying to do this when mochitest is running.
> > 
> > No, but when, I meant do you do it after frame construction, or during it.
> 
> If I would know. How can know this?

Not sure.  Maybe we can make a guess by looking at the call stack?  Or you could step into the frame constructor code, although that's not going to be fun.  Or if all else fails, maybe someone can point you at a better way of telling that.  roc, or bz maybe?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> > > No, but when, I meant do you do it after frame construction, or during it.
> > 
> > If I would know. How can know this?
> 
> Not sure.  Maybe we can make a guess by looking at the call stack? 

The a11y method (where I see null frame for text node of HTML input) is called from script, I'm not sure if call stack is useful here.

Ehsan, technically I think it doesn't make sense to keep this text node for a11y. Because if editor is not initalized then we have text node (which is accessible but can have null frame), if it's initialized then we see br node (which is not accessible). Can we change nsIContent::GetChildren() to ignore these nodes. Is there easy way to know whether text node is normal, whether it's bogus one?

Comment 15

[Boris Zbarsky [:bzbarsky]]

We can of course tell from the callstack whether we're "currently" in frame construction....  What's the stack to this point?

Comment 16

[(no longer active)]

(In reply to comment #14)
> Ehsan, technically I think it doesn't make sense to keep this text node for
> a11y. Because if editor is not initalized then we have text node (which is
> accessible but can have null frame), if it's initialized then we see br node
> (which is not accessible). Can we change nsIContent::GetChildren() to ignore
> these nodes. Is there easy way to know whether text node is normal, whether
> it's bogus one?

Well, the concept of bogus nodes in the editor is a br node with this special attribute set:

<http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.h#90>

But I'm not sure what the bigger picture is here.  As I've always preached, you should not be making assumptions about how the native anonymous content tree for an element looks like.  But if you specify what you need to know exactly, I can probably suggest a workaround which handles both cases, when we have an editor, and when we don't.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> We can of course tell from the callstack whether we're "currently" in frame
> construction....  What's the stack to this point?

I afraid I can't reproduce this any more. I get a frame.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> But I'm not sure what the bigger picture is here.  As I've always preached, you
> should not be making assumptions about how the native anonymous content tree
> for an element looks like.  But if you specify what you need to know exactly, I
> can probably suggest a workaround which handles both cases, when we have an
> editor, and when we don't.

I think it would be great if we get text nodes for HTML input and textarea that present a value. If there is not value then we'll be fine if we get empty set of nodes.

Now we have anonymous content (skipping root div element) that we wouldn't like to see:
1) empty text node when editor is not initalized (accessible node)
2) br node when editor is initialized (not accessible node because it's handled specially in a11y)
3) placeholder text (if I get right it's about to go away and we need to figure out better solution than expose it as accessible nodes)

I think we might be allowed to hack nsIAnonymousCreater::GetAnonymousContent() since it was created specially for a11y, however it might considered as non desirable.

Comment 19

[alexander :surkov (:asurkov)]

Ehsan, let's move discussion into bug 567318. For this bug I'll add a hack to support empty inputs with unitialized editor.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 21

[Marco Zehe (:MarcoZ)]

Comment on attachment 446681 [details] [diff] [review]
patch3

r=me. Note I also took a look at the code part and am fine with it as well.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 446681 [details] [diff] [review]
patch3

(In reply to comment #21)
> (From update of attachment 446681 [details] [diff] [review])
> r=me. Note I also took a look at the code part and am fine with it as well.

Thanks, Marco, then I won't bother David.

Comment 23

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/fa47ae62e1e2

Comment 24

[(no longer active)]

(In reply to comment #18)
> I think it would be great if we get text nodes for HTML input and textarea that
> present a value. If there is not value then we'll be fine if we get empty set
> of nodes.
> 
> Now we have anonymous content (skipping root div element) that we wouldn't like
> to see:
> 1) empty text node when editor is not initalized (accessible node)

We can't remove the empty text node there for performance reasons.

> 2) br node when editor is initialized (not accessible node because it's handled
> specially in a11y)

This is bug 240933.

> 3) placeholder text (if I get right it's about to go away and we need to figure
> out better solution than expose it as accessible nodes)

It's not really going away.  It's only going away when there's no actual placeholder value set.  In other words, if you don't have a placeholder value, you shouldn't get a div and text node representing it.

> I think we might be allowed to hack nsIAnonymousCreater::GetAnonymousContent()
> since it was created specially for a11y, however it might considered as non
> desirable.

I'm not sure what you mean here.