This highlighter would display bounds and later relevant information for accessible objects laid out on the page. It should implement custom highlighter spec and should also work both in content and in chrome.

Comment on attachment 8941486 [details] [diff] [review] 1428431.patch Review of attachment 8941486 [details] [diff] [review]: ----------------------------------------------------------------- Passing this off to pbro

Comment on attachment 8941486 [details] [diff] [review] 1428431.patch Review of attachment 8941486 [details] [diff] [review]: ----------------------------------------------------------------- I made some comments, but I'd really like to test this somehow. I've tried instantiating the highlighter from scratchpad but had some problems and couldn't make it work. How are you testing it? Can you provide your test page/script/code, whatever will do, but I'd love to give this a try. ::: devtools/server/actors/highlighters.css @@ +648,5 @@ > + opacity: 0.6; > +} > + > +/* Box model regions can be faded (see the onlyRegionArea option in > + highlighters.js) in order to only display certain regions. */ The onlyRegionArea options doesn't exist in your highlighter. Is this a piece of copied CSS that needs cleaning up? ::: devtools/server/actors/highlighters/accessible.js @@ +34,5 @@ > + opacity: 0.6; > + }`); > + > +/** > + * A helper functino that calculate accessible object bounds and positioning to s/functino/function @@ +38,5 @@ > + * A helper functino that calculate accessible object bounds and positioning to > + * be used for highlighting. > + * @param {Object} win window that contains accessible object. > + * @param {Object} accessible AccessibleActor for which bounds are calculated. > + * @return {?JSON} Returns, if available, positioning and bounds nit: usually we give the type like this instead: {Object|null} @@ +90,5 @@ > + * > + * It is used by the AccessibleHighlighterActor when canvasframe-based > + * AccessibleHighlighter can't be used. This is the case for XUL windows. > + */ > +class SimpleAccessibleHighlighter { What characterize this highlighter isn't necessarily its simplicity. Maybe a better name would be XULWindowAccessibleHighlighter. @@ +151,5 @@ > + * @return {?JSON} Returns, if available, positioning and bounds > + * information for the accessible object. > + */ > + get _bounds() { > + return getBounds(this.win, this.options.accessible); this.options is an object that is created by deserializing JSON sent with the show request. What do you expect the accessible option to be here? I don't understand how this can work. @@ +172,5 @@ > + let isSameAccessible = this.options && > + this.options.accessible === options.accessible; > + > + if (!isNodeValid(node) || (isSameNode && isSameAccessible)) { > + return false; Maybe also return false if this.options.accessible is undefined, since it seems to be mandatory here. @@ +180,5 @@ > + this.currentNode = node; > + > + let shown = this._show(); > + if (shown) { > + this.emit("shown"); Is this used somewhere? Since this patch only contains this highlighter, I don't know whether this event is supposed to be used from somewhere. Depending on where you intend to use it, maybe you could switch to the event-emitter instead of using the old-event-emitter. (same in other places of the code). @@ +193,5 @@ > + */ > + _show() { > + if (this._highlightTimer) { > + clearTimeout(this._highlightTimer); > + this._highlightTimer = null; You also need to do this in the destroy function. Just in case. @@ +199,5 @@ > + > + let shown = this._update(); > + this.emit("ready"); > + > + let { duration } = this.options; The duration option should also be documented in the jsdoc of the show method above. @@ +216,5 @@ > + */ > + _update() { > + let bounds = this._bounds; > + if (!bounds) { > + this._hideAccessibleBounds(); You can also move this to the very top of the function, because you need to hide here, and also further below, so at least this would be done once and for all at the start. @@ +386,5 @@ > + /** > + * Destroy the nodes. Remove listeners. > + */ > + destroy() { > + this.highlighterEnv.off("will-navigate", this.onWillNavigate); destroy is also a good place to clear the timeout in case it's still running. @@ +480,5 @@ > + * @return {?JSON} Returns, if available, positioning and bounds > + * information for the accessible object. > + */ > + get _bounds() { > + return getBounds(this.win, this.options.accessible); Same comment as before in the other highlighter. What is the type of this.options.accessible that you expect here? If it's an actorID, you'll probably need to call getActor on it.

Also based on Gabriel's feedback, split 2 highlighters into separate files. For tests and usage see bug 1428430. Thanks!

Comment on attachment 8943489 [details] [diff] [review] 1428431 patch v2 Review of attachment 8943489 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/xul-accessible.js @@ +38,5 @@ > +class XULWindowAccessibleHighlighter { > + constructor(highlighterEnv) { > + this.highlighterEnv = highlighterEnv; > + this.win = highlighterEnv.window; > + this.ID_CLASS_PREFIX = "accessible-"; This prefix isn't very useful in that particular case, because we're not using the canvasFrame anonymous content API here. You have direct access to your DOM nodes, so I think you should remove this prefix and not use it from _buildMarkup. @@ +98,5 @@ > + * @return {Object|null} Returns, if available, positioning and bounds > + * information for the accessible object. > + */ > + get _bounds() { > + return getBounds(this.win, this.options.accessible); I still don't understand how you intend to make this work. this.options is a JSON type object that is being sent as part of the show request for all highlighters. It goes through the protocol transport, and as such, gets stringified. Because it is defined as being JSON, the protocol will simply do a JSON.stringify on it. It looks like here, you expect it to be an AccessibleActor of some kind, so an object. If you want to pass an actor, you'll need to pass the actorID when sending the options from the client-side. And then, upon receiving this data on the server, you'll need to transform that ID into the corresponding actor again with: this.conn.getActor(this.options.accessible);

Sorry Yura, I was a bit too fast at cancelling this review. I now realize that there's more code in that other bug. And I know understand how you intend to be using the highlighter. I do have a question though: why not go through our CustomHighlighter API like for all other highlighters? Here, you are exposing a getHighlighter method on your walker, right? And that, in turn creates a AccessibleHighlighterActor, which then replicate the same API that our CustomHighlighterActor already has. I think you might be able to simplify things a bit if you went and used that thing instead. It is exposed on the client-side via toolbox.highlighterUtils.getHighlighterByType. This way, you wouldn't need a getHighlighter method, nor an AccessibleHighlighterActor at all. Of course, you would still have to deal with switching between the XULWindow and the HTMLWindow versions of the your highlighters. But I think you could do this client-side too: test whether the panel is running in the browser toolbox, and if so, then use getHighlighterByType with the XUL version, otherwise, with the normal version. I think toolbox.target.chrome should be enough to tell you whether you're in browser toolbox mode or not.

(In reply to Patrick Brosset <:pbro> from comment #6) > Sorry Yura, I was a bit too fast at cancelling this review. I now realize > that there's more code in that other bug. > And I know understand how you intend to be using the highlighter. > > I do have a question though: why not go through our CustomHighlighter API > like for all other highlighters? > Here, you are exposing a getHighlighter method on your walker, right? And > that, in turn creates a AccessibleHighlighterActor, which then replicate the > same API that our CustomHighlighterActor already has. > > I think you might be able to simplify things a bit if you went and used that > thing instead. > It is exposed on the client-side via > toolbox.highlighterUtils.getHighlighterByType. > This way, you wouldn't need a getHighlighter method, nor an > AccessibleHighlighterActor at all. > > Of course, you would still have to deal with switching between the XULWindow > and the HTMLWindow versions of the your highlighters. > But I think you could do this client-side too: test whether the panel is > running in the browser toolbox, and if so, then use getHighlighterByType > with the XUL version, otherwise, with the normal version. > I think toolbox.target.chrome should be enough to tell you whether you're in > browser toolbox mode or not. Hi Patrick, thanks for the feedback. Just a follow up question: the main blocker for what you suggested (I think) is that CustomHighlighterActor assumes that it needs a canvasframe, e.g. not in xul window. HighlighterActor on the other hand allows for simple-outline highlighter in XUL which is sort of similar to what I need. You think extending current custom highlighter actor would be reasonable then?

(In reply to Yura Zenevich [:yzen] from comment #7) > (In reply to Patrick Brosset <:pbro> from comment #6) > > Sorry Yura, I was a bit too fast at cancelling this review. I now realize > > that there's more code in that other bug. > > And I know understand how you intend to be using the highlighter. > > > > I do have a question though: why not go through our CustomHighlighter API > > like for all other highlighters? > > Here, you are exposing a getHighlighter method on your walker, right? And > > that, in turn creates a AccessibleHighlighterActor, which then replicate the > > same API that our CustomHighlighterActor already has. > > > > I think you might be able to simplify things a bit if you went and used that > > thing instead. > > It is exposed on the client-side via > > toolbox.highlighterUtils.getHighlighterByType. > > This way, you wouldn't need a getHighlighter method, nor an > > AccessibleHighlighterActor at all. > > > > Of course, you would still have to deal with switching between the XULWindow > > and the HTMLWindow versions of the your highlighters. > > But I think you could do this client-side too: test whether the panel is > > running in the browser toolbox, and if so, then use getHighlighterByType > > with the XUL version, otherwise, with the normal version. > > I think toolbox.target.chrome should be enough to tell you whether you're in > > browser toolbox mode or not. > > Hi Patrick, thanks for the feedback. Just a follow up question: the main > blocker for what you suggested (I think) is that CustomHighlighterActor > assumes that it needs a canvasframe, e.g. not in xul window. > HighlighterActor on the other hand allows for simple-outline highlighter in > XUL which is sort of similar to what I need. You think extending current > custom highlighter actor would be reasonable then? That's in relation to the XUL version custom highlighter.

I wonder how much CustomHighlighterActor really depends on the CanvasFrame being available. I don't think it does. I think one solution is to modify CustomHighlighterActor so it doesn't bail out when the current window is XUL (and fix the impacts this will cause on the Style-Editor, which depends on this). The other solution would be to modify your AccessibleHighlighterActor so it extends the CustomHighlighterActor so you can reduce the amount of code that you had to duplicate. Since you'd extend from it, you could only override its initialize method and do your own thing there. But I think the first solution might be better. If it can work, then it means we can use the CustomHighlighterActor (via toolbox.getHighlighterByType) with any type of highlighter, whether they use the CanvasFrame API or not.

Thinking about this more: you also need the instance of the highlighter on the server-side don't you? For your walker actor's pick method. So far, I've been trying to fit your highlighter workflow into our existing system, in order to enhance consistency and avoid having to write more unnecessary code. But your use case is evidently different because of this. So, perhaps the way you've been approaching this was the right way: handle the highlighter creation on the actor-side, so you have it there for picking accessible objects (and it's easy to deal with XUL vs. non-XUL), and expose a getHighlighter method for the client to still be able to get a reference to it and highlight stuff when needed. Having said that, the duplication between AccessibleHighlighterActor and CustomHighlighteActor still needs to be addressed if possible. I'll add comments regarding this in my review of Bug 1428430.

Comment on attachment 8947500 [details] [diff] [review] 1428431 patch v3 Review of attachment 8947500 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/accessible.js @@ +26,5 @@ > + * h.destroy(); > + * > + * Available options: > + * - accessible {nsIAccessible} Accessible object that contains all the > + * relevant bounds information. Here's a suggestion: let's turn this option into bounds. This would be an object with x, y, w, h properties (being Numbers). This way, no need to make this class deal with nsIAccessible objects at all. Instead, it would just accept these coordinates, and then convert them to the right values with your getBounds() utility (to take zoom, etc. into account). This would allow the highlighter to also be used from the client-side. If the client has these bounds, it will be able to use this highlighter directly too, which makes it more useful. Of course, this means that your AccessibleWalkerActor will need to pass x,y,w,h instead of an nsIAccessible object, but that seems pretty easy. ::: devtools/server/actors/highlighters/xul-accessible.js @@ +38,5 @@ > +class XULWindowAccessibleHighlighter { > + constructor(highlighterEnv) { > + this.highlighterEnv = highlighterEnv; > + this.win = highlighterEnv.window; > + this.ID_CLASS_PREFIX = "accessible-"; Is this prefix really needed? I don't think it is. We use this prefix technique in highlighters that use the CanvasFrame API because that's where we insert all our other highlighters, so IDs and classes need to be unique. Furthermore, here you just keep references to your DOM nodes, so there isn't even really a need for getElement either. @@ +45,5 @@ > + /** > + * Static getter that indicates that XULWindowAccessibleHighlighter supports > + * highlighting in XUL windows. > + */ > + static get XULSupported() { Is this getter used anywhere? It doesn't seem to be used in this patch, and I think the other code you're writing in the other bug doesn't use it either. @@ +66,5 @@ > + "role": "presentation" > + } > + }); > + > + let root = createNode(this.win, { Is this root element needed at all? It doesn't seem to be playing any role at all. @@ +79,5 @@ > + > + let bounds = createNode(this.win, { > + parent: root, > + attributes: { > + "class": "bounds", Let's get rid of the prefix, and just name the class "accessible-bounds" directly. @@ +95,5 @@ > + * @param {String} id > + * Highlighter markup elemet key. > + * @return {DOMNode} Element in the highlighter markup. > + */ > + getElement(id) { You therefore can remove this function.

Hopefully taken care of the comments.

Comment on attachment 8948497 [details] [diff] [review] 1428431 patch v4 Review of attachment 8948497 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for addressing my comments Yura. Looks good now.

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7e769b5942 added accessible highlighter classes for content and chrome docs. r=pbro

Hrm, I guess there was nothing to mention in the docs about this after all. Nothing yet, anyway. Let me know if I'm wrong ;-)