Closed Bug 838508 Opened 7 years ago Closed 7 years ago

add a text attributes inspection tab to accessible properties viewer

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #710558 - Flags: review?(neil)
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.
Attachment #710558 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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?
(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.
(In reply to neil@parkwaycc.co.uk 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
Attached patch patch2 (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #710558 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #711236 - Flags: review?(neil)
Sadly the mockup was harder to create than I expected...
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.
(In reply to neil@parkwaycc.co.uk 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?
(In reply to neil@parkwaycc.co.uk 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
(In reply to alexander :surkov from comment #9)
> (In reply to neil@parkwaycc.co.uk 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
(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.
Attached patch patch3 (obsolete) — Splinter Review
Attachment #711236 - Attachment is obsolete: true
Attachment #711236 - Flags: review?(neil)
Attachment #712227 - Flags: review?(neil)
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/>.
(In reply to neil@parkwaycc.co.uk 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?
(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.)
(In reply to neil@parkwaycc.co.uk 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?
(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.
Attached patch patchSplinter Review
Attachment #712227 - Attachment is obsolete: true
Attachment #712227 - Flags: review?(neil)
Attachment #712756 - Flags: review?(neil)
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.
Attachment #712756 - Flags: review?(neil) → review+
(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.)
(In reply to neil@parkwaycc.co.uk 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
http://hg.mozilla.org/dom-inspector/rev/70422bf408f2
Thank you for review!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: DOMi2.0.16
> 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
Blocks: DOMi2.0.15
No longer blocks: DOMi2.0.16
You need to log in before you can comment on or make changes to this bug.