838508 - add a text attributes inspection tab to accessible properties viewer

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[[e-mail]]

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

>+      var description = document.createElement("description");
>+      description.setAttribute("class", "textAttrsTextRange");
You're putting this inside a xul:description element, but you probably want to use an html element in this case, so that the text will wrap properly.

>+      this.mTextAttrObjs[description] = {
This only ever worked on debug builds before bz converted XULElement to webidl. Now it doesn't work at all*. Instead, just set the three values as expando properties on the element.

>+      var text = aAccessible.getText(offset, endOffset.value);
When the substring is itself accessible (e.g. <A>) then you simply see an object replacement character instead. I found it works for <B> though.

>+      description.addEventListener("click", this, false);
Inaccessible. (Sorry!) But it might work if you used an html:a element. (You'd still need :focus rules of course!)

>+  padding: 7px;
>+  border-bottom-left-radius: 2em;
>+  border-bottom-right-radius: 2em;
Weird juxtaposition of px and em. Were you after a particular effect?

>+  border-bottom: medium dotted Red;
>+  border-bottom: medium dotted MediumOrchid;
>+  border-bottom: medium groove Blue;
Any particular reasoning behind the choice of colours?

* Except in a build I have somewhere were I reinserted the debug pointer value so that I could compare DOM nodes based on their string representation in DOM Inspector or the Error Console.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #1)

> >+      var description = document.createElement("description");
> >+      description.setAttribute("class", "textAttrsTextRange");
> You're putting this inside a xul:description element, but you probably want
> to use an html element in this case, so that the text will wrap properly.

description having a text inside support wrapping

> >+      this.mTextAttrObjs[description] = {
> This only ever worked on debug builds before bz converted XULElement to
> webidl. Now it doesn't work at all*. Instead, just set the three values as
> expando properties on the element.

I don't really know how js is implemented but I'm curious why this statement tends to change anything on the element. I have one week ago build, I though bz landed that change a little bit early.

> >+      var text = aAccessible.getText(offset, endOffset.value);
> When the substring is itself accessible (e.g. <A>) then you simply see an
> object replacement character instead. I found it works for <B> though.

correct

> >+      description.addEventListener("click", this, false);
> Inaccessible. (Sorry!) But it might work if you used an html:a element.
> (You'd still need :focus rules of course!)

tabindex should do a magic if you want

> >+  padding: 7px;
> >+  border-bottom-left-radius: 2em;
> >+  border-bottom-right-radius: 2em;
> Weird juxtaposition of px and em. Were you after a particular effect?

I just copied/pasted from somewhere

> >+  border-bottom: medium dotted Red;
> >+  border-bottom: medium dotted MediumOrchid;
> >+  border-bottom: medium groove Blue;
> Any particular reasoning behind the choice of colours?

no, are there suggestions?

Comment 3

[[e-mail]]

(In reply to alexander surkov from comment #2)
> (In reply to comment #1)
> > (From update of attachment 710558 [details] [diff] [review])
> > >+      var description = document.createElement("description");
> > >+      description.setAttribute("class", "textAttrsTextRange");
> > You're putting this inside a xul:description element, but you probably want
> > to use an html element in this case, so that the text will wrap properly.
> description having a text inside support wrapping
Only within the description element itself, i.e.
[text] [more text]
[this text is very long, so it wraps, but only
once it's forced itself on to a new line.]
[short text again]
If you switch to HTML children you get
[text] [more text] [this text is very long
but it wraps in line] [short text again]

> > >+      this.mTextAttrObjs[description] = {
> > This only ever worked on debug builds before bz converted XULElement to
> > webidl. Now it doesn't work at all*. Instead, just set the three values as
> > expando properties on the element.
> I don't really know how js is implemented but I'm curious why this statement
> tends to change anything on the element. I have one week ago build, I though
> bz landed that change a little bit early.
Property names are strings. description isn't a string, it's a XUL element. This means that it gets stringified before it's used as a property name. In debug builds of Gecko 20 and earlier this would be a unique string, because it contained some raw pointer values. These days it is not, and all of your objects end up overwriting each other.

> > >+      var text = aAccessible.getText(offset, endOffset.value);
> > When the substring is itself accessible (e.g. <A>) then you simply see an
> > object replacement character instead. I found it works for <B> though.
> correct
Well, it's possibly confusing and/or unhelpful.

> > >+      description.addEventListener("click", this, false);
> > Inaccessible. (Sorry!) But it might work if you used an html:a element.
> > (You'd still need :focus rules of course!)
> tabindex should do a magic if you want
Well, not enough magic on its own.

> > >+  padding: 7px;
> > >+  border-bottom-left-radius: 2em;
> > >+  border-bottom-right-radius: 2em;
> > Weird juxtaposition of px and em. Were you after a particular effect?
> I just copied/pasted from somewhere
If you can remember where, that would be great ;-)

> > >+  border-bottom: medium dotted Red;
> > >+  border-bottom: medium dotted MediumOrchid;
> > >+  border-bottom: medium groove Blue;
> > Any particular reasoning behind the choice of colours?
> no, are there suggestions?
blue, red and purple.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #3)
> > description having a text inside support wrapping
> Only within the description element itself, i.e.
> [text] [more text]
> [this text is very long, so it wraps, but only
> once it's forced itself on to a new line.]
> [short text again]
> If you switch to HTML children you get
> [text] [more text] [this text is very long
> but it wraps in line] [short text again]

I believe I get the second case having descriptions, I put a text content into nested description as well.

> > > >+      this.mTextAttrObjs[description] = {
> > > This only ever worked on debug builds before bz converted XULElement to
> > > webidl. Now it doesn't work at all*. Instead, just set the three values as
> > > expando properties on the element.
> > I don't really know how js is implemented but I'm curious why this statement
> > tends to change anything on the element. I have one week ago build, I though
> > bz landed that change a little bit early.
> Property names are strings. description isn't a string, it's a XUL element.
> This means that it gets stringified before it's used as a property name. In
> debug builds of Gecko 20 and earlier this would be a unique string, because
> it contained some raw pointer values. These days it is not, and all of your
> objects end up overwriting each other.

ok, I will give them ids and switch to them

> > > >+      var text = aAccessible.getText(offset, endOffset.value);
> > > When the substring is itself accessible (e.g. <A>) then you simply see an
> > > object replacement character instead. I found it works for <B> though.
> > correct
> Well, it's possibly confusing and/or unhelpful.

that's ok, embedded characters is what AT deals with, thus we need to inspect them

> > > >+      description.addEventListener("click", this, false);
> > > Inaccessible. (Sorry!) But it might work if you used an html:a element.
> > > (You'd still need :focus rules of course!)
> > tabindex should do a magic if you want
> Well, not enough magic on its own.

true

> > > >+  padding: 7px;
> > > >+  border-bottom-left-radius: 2em;
> > > >+  border-bottom-right-radius: 2em;
> > > Weird juxtaposition of px and em. Were you after a particular effect?
> > I just copied/pasted from somewhere
> If you can remember where, that would be great ;-)

I think I played with styles on w3schools, they have basic examples. but why?

> > > >+  border-bottom: medium dotted Red;
> > > >+  border-bottom: medium dotted MediumOrchid;
> > > >+  border-bottom: medium groove Blue;
> > > Any particular reasoning behind the choice of colours?
> > no, are there suggestions?
> blue, red and purple.

ok

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 6

[[e-mail]]

Attachment: Mockup of "really" inline text.

Sadly the mockup was harder to create than I expected...

Comment 7

[[e-mail]]

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

>+      var endOffset = { };
>+      var textAttrObj = {
>+        attrs: aAccessible.getTextAttributes(false, offset, { }, endOffset),
>+        startOffset: offset,
>+        endOffset: endOffset.value
>+      };
[This would probably be undefined in C++!]

>+      description.setAttribute("textoffset", offset);
>+      this.mTextAttrObjs[offset] = textAttrObj;
I guess this works but that's still overkill, just set it as a property.

>+      description.addEventListener("click", this, false);
The mouse down focuses the description so you don't need to listen for clicks.

>+      description.setAttribute("tabindex", "1");
-moz-user-focus makes XUL elements tabbable, so you don't need tabindex.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #7)
> >+      description.setAttribute("textoffset", offset);
> >+      this.mTextAttrObjs[offset] = textAttrObj;
> I guess this works but that's still overkill, just set it as a property.

Can you give a code snippet please? I'm not sure I follow.

> >+      description.addEventListener("click", this, false);
> The mouse down focuses the description so you don't need to listen for
> clicks.
> 
> >+      description.setAttribute("tabindex", "1");
> -moz-user-focus makes XUL elements tabbable, so you don't need tabindex.

ok

btw, would it better to have dotted outline when item is focused?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #6)
> Created attachment 711299 [details]
> Mockup of "really" inline text.
> 
> Sadly the mockup was harder to create than I expected...

I see the difference, thank you

Comment 10

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #9)
> (In reply to [e-mail] from comment #6)
> > Created attachment 711299 [details]
> > Mockup of "really" inline text.
> > 
> > Sadly the mockup was harder to create than I expected...
> 
> I see the difference, thank you

btw, can you give me a markup you used, HTML p + span gives overlapping picture

Comment 11

[[e-mail]]

(In reply to alexander surkov from comment #8)
> (In reply to comment #7)
> > >+      description.setAttribute("textoffset", offset);
> > >+      this.mTextAttrObjs[offset] = textAttrObj;
> > I guess this works but that's still overkill, just set it as a property.
> Can you give a code snippet please? I'm not sure I follow.
description.textAttributes =
    aAccessible.getTextAttributes(false, offset, { }, endOffset);
description.startOffset = offset;
description.endOffset = endOffset.value;

> btw, would it better to have dotted outline when item is focused?
Well, the purple highlight tracks the focus, so that helps.

(In reply to alexander surkov from comment #10)
> HTML p + span gives overlapping
I couldn't think of anything better than a line height of 200% for my mockup (which just used span, no p) although jfkthame thinks that's correct.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 13

[[e-mail]]

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

>+      var description = document.createElementNS(kHTMLNS, "span");
It's not a description element any more ;-)

>+      description.setAttribute("textoffset", offset);
>+      this.mTextAttrObjs[offset] = textAttrObj;
So, you didn't understand my example?

>+        <html:p id="textAttrs:content"/>
This on the other hand should stay as a <description/>.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #13)
> Comment on attachment 712227 [details] [diff] [review]
> patch3
> 
> >+      var description = document.createElementNS(kHTMLNS, "span");
> It's not a description element any more ;-)
> 
> >+      description.setAttribute("textoffset", offset);
> >+      this.mTextAttrObjs[offset] = textAttrObj;
> So, you didn't understand my example?

yes, I said that in comment #8

> >+        <html:p id="textAttrs:content"/>
> This on the other hand should stay as a <description/>.

I don't mind, is there a difference?

Comment 15

[[e-mail]]

(In reply to alexander surkov from comment #14)
> (In reply to comment #13)
> > (From update of attachment 712227 [details] [diff] [review])
> > >+      description.setAttribute("textoffset", offset);
> > >+      this.mTextAttrObjs[offset] = textAttrObj;
> > So, you didn't understand my example?
> yes, I said that in comment #8
I gave you a second example in comment #11

> > >+        <html:p id="textAttrs:content"/>
> > This on the other hand should stay as a <description/>.
> I don't mind, is there a difference?
<p> only works "by luck" rather than design, while <label> and <description> are designed to contain HTML within XUL. (Obviously most of the time people only use them because, being HTML, the text wraps automatically.)

Comment 16

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #11)
> (In reply to alexander surkov from comment #8)
> > (In reply to comment #7)
> > > >+      description.setAttribute("textoffset", offset);
> > > >+      this.mTextAttrObjs[offset] = textAttrObj;
> > > I guess this works but that's still overkill, just set it as a property.
> > Can you give a code snippet please? I'm not sure I follow.
> description.textAttributes =
>     aAccessible.getTextAttributes(false, offset, { }, endOffset);
> description.startOffset = offset;
> description.endOffset = endOffset.value;

due to some reason I missed this comment, sorry.

I'm pretty sure I had a bug in DOMi related with this approach. It was related with not uniqueness of JS wrapper of c++ object. Say, if I set a property to c++ object and gets this object from event under certain circumstances then that c++ doesn't have that property. I assume that's not something I should care nowdays?

Comment 17

[[e-mail]]

(In reply to alexander surkov from comment #16)
> I'm pretty sure I had a bug in DOMi related with this approach. It was
> related with not uniqueness of JS wrapper of c++ object. Say, if I set a
> property to c++ object and gets this object from event under certain
> circumstances then that c++ doesn't have that property. I assume that's not
> something I should care nowdays?
As far as I know, C++ objects fall into three categories:
1. Native wrapper. On these objects, attempting to set a non-existing property throws an "invalid operation on wrapped native" exception. These objects can only be accessed from chrome unless they are a security checked component.
2. Object wrapper. The wrapper looks like a JavaScript object but it is garbage collected, so its properties can vanish without warning.
3. DOM. These objects preserve their wrapper so that it exists as long as the element exists. However this was prone to leaks (e.g. setting an element as a property of a child element) which is why the cycle collector was invented.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 19

[[e-mail]]

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

>+      textRangeElm.setAttribute("tabindex", 1);
Please use 0 as 1 takes it out of the natural tab order.

> <page id="winAccessibleProps"
>-      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+      xmlns:html="http://www.w3.org/1999/xhtml">
Don't need this change any more.

Comment 20

[[e-mail]]

(In reply to alexander surkov from comment #8)
> btw, would it better to have dotted outline when item is focused?
Looks like <html:span tabindex> gives you a dotted outline for free.

(In reply to comment #1)
> it might work if you used an html:a element.
Apparently it's not enough on its own, because that's an anchor, not a link, so it isn't clickable in the activation sense. (Adding an href would make it clickable, but that's silly.)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #20)
> (In reply to alexander surkov from comment #8)
> > btw, would it better to have dotted outline when item is focused?
> Looks like <html:span tabindex> gives you a dotted outline for free.

yes

Comment 22

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/dom-inspector/rev/70422bf408f2
Thank you for review!

Comment 23

[Ian Neal]

Added to sk locale in:
http://hg.mozilla.org/dom-inspector/rev/130773eb1a64
Added to ru locale in:
http://hg.mozilla.org/dom-inspector/rev/f99f5c9c26e9
Added to en-GB locale in:
http://hg.mozilla.org/dom-inspector/rev/f3f70eccc305
http://hg.mozilla.org/dom-inspector/rev/a97bec13e961

de and pl locales still need it adding.

Comment 24

[Philip Chee]

> de and pl locales still need it adding.

Added to de locale in:
http://hg.mozilla.org/dom-inspector/rev/8306052087a9
Added to pl locale in:
http://hg.mozilla.org/dom-inspector/rev/c218f0f2ae41