Closed Bug 1266456 Opened 4 years ago Closed 4 years ago

Create a HTML replacement for the autocomplete suggestion popup

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox48 affected, firefox49 affected, firefox50 fixed)

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox48 --- affected
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [btpp-fix-later] [devtools-html])

Attachments

(4 files, 7 obsolete files)

58 bytes, text/x-review-board-request
bgrins
: review+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
Details
The autocomplete-popup used in devtools is currently relying on XUL panel and XUL elements and should use a HTML tooltip from Bug 1259834 instead. 

The autocomplete popup is used in many panels of the devtools (html search, inplace editor, webconsole etc...).

This bug is about implementing the popup content. Displaying and positioning the popup will use the outcome of Bug 1259834.
Depends on: 1259834
Whiteboard: [btpp-fix-later] [devtools-html]
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Iteration: 49.3 - Jun 6 → 50.1
Iteration: 50.1 → 50.2
Review commit: https://reviewboard.mozilla.org/r/59756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59756/
Attachment #8764249 - Flags: review?(jsnajdr)
Attachment #8764250 - Flags: review?(jsnajdr)
Attachment #8764251 - Flags: review?(jsnajdr)
Attachment #8764252 - Flags: review?(poirot.alex)
Attachment #8764253 - Flags: review?(bgrinstead)
Attachment #8764254 - Flags: review?(bgrinstead)
Attachment #8764255 - Flags: review?(jsnajdr)
setContent expects 3 arguments: content, width, height. Height is already optional
but for the autocomplete migration, the width will also become optional.

Using an object argument for width and height makes this easier.

Review commit: https://reviewboard.mozilla.org/r/59758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59758/
When calling HTMLTooltip::show() consecutively, we were leaking a window object.
The timeout responsible for attaching the click event on window is now cleared
before attaching a new one.

Review commit: https://reviewboard.mozilla.org/r/59760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59760/
Some test documents are using a <page> object instead of a <window> object.
When inserting the tooltip container  inside of the document, the HTMLTooltip
will fallback to <page> if <window> can not be found.

Review commit: https://reviewboard.mozilla.org/r/59762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59762/
The autocomplete popup defines its width by finding the longest label
to display and then applying a "width:Xch" to its content, where X is
the length of the longest label + 3.

In order to support this, the HTMLTooltip setContent() methods allows to
use width: "auto" (which also becomes the default value). In this case,
the HTMLTooltip show() method will automatically compute the preferred
width for the tooltip. It will first calculate the tooltip height, then
measure the width of the tooltip for this computed height and use it as
the preferred width.

Review commit: https://reviewboard.mozilla.org/r/59764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59764/
The autocomplete popup is displayed relatively to an anchor, but will be
shifted on the x-axis in order to be aligned with the word being completed.

To support this, the HTMLTooltip show() method now accepts x and y offsets.

Review commit: https://reviewboard.mozilla.org/r/59766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59766/
Attachment #8764252 - Flags: review?(poirot.alex) → review+
Comment on attachment 8764252 [details]
Bug 1266456 - part4: HTMLTooltip support XUL docs using page instead of window;

https://reviewboard.mozilla.org/r/59762/#review57082

::: devtools/client/shared/widgets/HTMLTooltip.js:81
(Diff revision 1)
>  
>    this.container = this._createContainer();
>  
>    if (this._isXUL()) {
> -    this.doc.querySelector("window").appendChild(this.container);
> +    let parent = this.doc.querySelector("window") || this.doc.querySelector("page");
> +    parent.appendChild(this.container);

It looks like you just want:
this.doc.documentElement.appendChild(this.container);
Comment on attachment 8764249 [details]
Bug 1266456 - part1: HTMLTooltip cleanup test & jsdoc for synchronous setContent;

https://reviewboard.mozilla.org/r/59756/#review57256
Attachment #8764249 - Flags: review?(jsnajdr) → review+
https://reviewboard.mozilla.org/r/59758/#review57268

Looks good, but are you sure you updated all tooltip.setContent call sites? For example, the one in netmonitor-view.js still uses the old signature.

::: devtools/client/shared/widgets/HTMLTooltip.js:116
(Diff revision 1)
> -   * @param {Number} width
> -   *        Preferred width for the tooltip container
> -   * @param {Number} height (optional)
> -   *        Preferred height for the tooltip container. If the content height is
> -   *        smaller than the container's height, the tooltip will automatically
> -   *        shrink around the content. If not specified, will use all the height
> +   * @param {Object}
> +   *        - {Number} width: preferred width for the tooltip container
> +   *        - {Number} height: optional, preferred height for the tooltip container. If
> +   *          the content height is smaller than the container's height, the tooltip will
> +   *          automatically shrink around the content. If not specified, height will
> +   *          default to Infinity and the tooltip will use all the height available.

This effectively behaves like max-height, right? I think the documentation would be clearer if it mentioned that the property is a maximum height of the content.

Also, if not already done in one of the other commits, let's add documentation for the width: 'auto' option.
(In reply to Jarda Snajdr [:jsnajdr] from comment #13)
> https://reviewboard.mozilla.org/r/59758/#review57268
> 
> Looks good, but are you sure you updated all tooltip.setContent call sites?
> For example, the one in netmonitor-view.js still uses the old signature.
> 

Good catch! I missed this one after rebasing on top of the stacktraces tooltip. All other call sites have already been updated.

> ::: devtools/client/shared/widgets/HTMLTooltip.js:116
> (Diff revision 1)
> > -   * @param {Number} width
> > -   *        Preferred width for the tooltip container
> > -   * @param {Number} height (optional)
> > -   *        Preferred height for the tooltip container. If the content height is
> > -   *        smaller than the container's height, the tooltip will automatically
> > -   *        shrink around the content. If not specified, will use all the height
> > +   * @param {Object}
> > +   *        - {Number} width: preferred width for the tooltip container
> > +   *        - {Number} height: optional, preferred height for the tooltip container. If
> > +   *          the content height is smaller than the container's height, the tooltip will
> > +   *          automatically shrink around the content. If not specified, height will
> > +   *          default to Infinity and the tooltip will use all the height available.
> 
> This effectively behaves like max-height, right? I think the documentation
> would be clearer if it mentioned that the property is a maximum height of
> the content.
> 

Yes it is like max-height, I will add it to the comment, thanks!

> Also, if not already done in one of the other commits, let's add
> documentation for the width: 'auto' option.

The "auto" value is not added until part5, but even there, the documentation is missing and I will add it.
Comment on attachment 8764249 [details]
Bug 1266456 - part1: HTMLTooltip cleanup test & jsdoc for synchronous setContent;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59756/diff/1-2/
Comment on attachment 8764250 [details]
Bug 1266456 - part2: HTMLTooltip setContent() use object as 2nd arg;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59758/diff/1-2/
Comment on attachment 8764251 [details]
Bug 1266456 - part3: HTMLTooltip show() clear timeout to avoid window leaks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59760/diff/1-2/
Comment on attachment 8764252 [details]
Bug 1266456 - part4: HTMLTooltip support XUL docs using page instead of window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59762/diff/1-2/
Comment on attachment 8764253 [details]
Bug 1266456 - part5: HTMLTooltip setContent() support "auto" width parameter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59764/diff/1-2/
Comment on attachment 8764254 [details]
Bug 1266456 - part6: HTMLTooltip show() now accepts x,y offsets;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59766/diff/1-2/
Comment on attachment 8764255 [details]
Bug 1266456 - part7: tooltips.css remove unused event details tooltip rule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59768/diff/1-2/
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/1-2/
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/1-2/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/1-2/
https://reviewboard.mozilla.org/r/59762/#review57082

> It looks like you just want:
> this.doc.documentElement.appendChild(this.container);

I didn't know I could simply append it to the document element here. Thanks a lot!
Comment on attachment 8764253 [details]
Bug 1266456 - part5: HTMLTooltip setContent() support "auto" width parameter;

https://reviewboard.mozilla.org/r/59764/#review57302

::: devtools/client/shared/widgets/HTMLTooltip.js:156
(Diff revision 2)
> + */
> +const getRelativeRect = function (node, relativeTo) {
> +  // Width and Height can be taken from the rect.
> +  let {width, height} = node.getBoundingClientRect();
> +
> +  let quads = node.getBoxQuads({relativeTo});

I see this line is moved so don't need to update it here, but doesn't boxQuads return all of this information?  The bounds has top/left/bottom/right/width/height.  Or is it different in some cases from getBoundingClientRect?
Attachment #8764253 - Flags: review?(bgrinstead) → review+
Comment on attachment 8764254 [details]
Bug 1266456 - part6: HTMLTooltip show() now accepts x,y offsets;

https://reviewboard.mozilla.org/r/59766/#review57120
Attachment #8764254 - Flags: review?(bgrinstead) → review+
Comment on attachment 8764250 [details]
Bug 1266456 - part2: HTMLTooltip setContent() use object as 2nd arg;

https://reviewboard.mozilla.org/r/59758/#review57378
Attachment #8764250 - Flags: review?(jsnajdr) → review+
Comment on attachment 8764255 [details]
Bug 1266456 - part7: tooltips.css remove unused event details tooltip rule;

https://reviewboard.mozilla.org/r/59768/#review57390

If I understand it correctly, the class .devtools-tooltip-events-container continues to be used and you're removing only the unused #id rule. If that's true, it's r+.
Attachment #8764255 - Flags: review?(jsnajdr) → review+
https://reviewboard.mozilla.org/r/59768/#review57390

Thanks for the review! Yes, .devtools-tooltip-events-container is still used, but #devtools-tooltip-events-container does not match any element.
Comment on attachment 8764251 [details]
Bug 1266456 - part3: HTMLTooltip show() clear timeout to avoid window leaks;

https://reviewboard.mozilla.org/r/59760/#review57406

::: devtools/client/shared/test/browser_html_tooltip_consecutive-show.js:32
(Diff revision 2)
> +loadHelperScript("helper_html_tooltip.js");
> +
> +function getTooltipContent(doc) {
> +  let div = doc.createElementNS(HTML_NS, "div");
> +  div.style.height = "50px";
> +  div.style.boxSizing = "border-box";

The box-sizing: border-box style is probably not necessary here and can be removed.
Attachment #8764251 - Flags: review?(jsnajdr) → review+
Comment on attachment 8764251 [details]
Bug 1266456 - part3: HTMLTooltip show() clear timeout to avoid window leaks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59760/diff/2-3/
Comment on attachment 8764252 [details]
Bug 1266456 - part4: HTMLTooltip support XUL docs using page instead of window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59762/diff/2-3/
Comment on attachment 8764253 [details]
Bug 1266456 - part5: HTMLTooltip setContent() support "auto" width parameter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59764/diff/2-3/
Comment on attachment 8764254 [details]
Bug 1266456 - part6: HTMLTooltip show() now accepts x,y offsets;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59766/diff/2-3/
Comment on attachment 8764255 [details]
Bug 1266456 - part7: tooltips.css remove unused event details tooltip rule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59768/diff/2-3/
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/2-3/
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/2-3/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/2-3/
https://reviewboard.mozilla.org/r/59760/#review57406

> The box-sizing: border-box style is probably not necessary here and can be removed.

Thanks for the reviews Jarda!
https://reviewboard.mozilla.org/r/59764/#review57302

> I see this line is moved so don't need to update it here, but doesn't boxQuads return all of this information?  The bounds has top/left/bottom/right/width/height.  Or is it different in some cases from getBoundingClientRect?

That's an interesting point actually. In case your anchor is displayed on several quads, getBoxQuads()[0].bounds.height will only return the height of the first quad, not of the whole element.

If the tooltip is displayed below the anchor and is using several quads, using the height from the quad will make the tooltip overlap the anchor. I figured it would be preferable to avoid overlaps as much as possible, which is why I use the BoudingClientRect height information.

I can add a comment to explain this.
Try push for patches 1 to 7: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db5b6a8f63b3

If try is green, I will land those in order to avoid make rebasing easier for Bug 1267403
try is green, landing parts 1-7.
Keywords: leave-open
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8a4f5f3b73e2
part1: HTMLTooltip cleanup test & jsdoc for synchronous setContent;r=jsnajdr
https://hg.mozilla.org/integration/fx-team/rev/682957e33397
part2: HTMLTooltip setContent() use object as 2nd arg;r=jsnajdr
https://hg.mozilla.org/integration/fx-team/rev/4f42717b9103
part3: HTMLTooltip show() clear timeout to avoid window leaks;r=jsnajdr
https://hg.mozilla.org/integration/fx-team/rev/b3e5f1ec1af3
part4: HTMLTooltip support XUL docs using page instead of window;r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/56aa4e3e6287
part5: HTMLTooltip setContent() support "auto" width parameter;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/88e3339aaec1
part6: HTMLTooltip show() now accepts x,y offsets;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/9688cac5afbe
part7: tooltips.css remove unused event details tooltip rule;r=jsnajdr
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/3-4/
Attachment #8764256 - Attachment description: Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css → Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;
Attachment #8764257 - Attachment description: Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup → Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;
Attachment #8764258 - Attachment description: Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration → Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;
Attachment #8764256 - Flags: review?(bgrinstead)
Attachment #8764257 - Flags: review?(bgrinstead)
Attachment #8764258 - Flags: review?(bgrinstead)
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/3-4/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/3-4/
Attachment #8764249 - Attachment is obsolete: true
Attachment #8764250 - Attachment is obsolete: true
Attachment #8764251 - Attachment is obsolete: true
Attachment #8764252 - Attachment is obsolete: true
Attachment #8764253 - Attachment is obsolete: true
Attachment #8764254 - Attachment is obsolete: true
Attachment #8764255 - Attachment is obsolete: true
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/4-5/
Fixed test failures on devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js. New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab9664aa50be
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

https://reviewboard.mozilla.org/r/59770/#review57996
Attachment #8764256 - Flags: review?(bgrinstead) → review+
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

https://reviewboard.mozilla.org/r/59774/#review58278

::: devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js:118
(Diff revision 5)
>  /**
>   * Get the autocomplete panel left offset, relative to the provided input's left offset.
>   */
>  function getPopupOffset({popup, input}) {
>    let popupQuads = popup._panel.getBoxQuads({relativeTo: input});
> +  console.log("popupQuads[0].bounds.left", popupQuads[0].bounds.left);

Unintentional console.log?

::: devtools/client/shared/test/helper_inplace_editor.js:32
(Diff revision 5)
>    let span = options.element = createSpan(doc);
>    if (textContent) {
>      span.textContent = textContent;
>    }
>  
> +  yield waitForTick();

Why's this needed?  Please add a comment
Attachment #8764258 - Flags: review?(bgrinstead) → review+
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

https://reviewboard.mozilla.org/r/59772/#review58162

This is sort of hard to review because it's a lot of code moving around.  I don't have a better suggestion on how to split it up though so I've focused on it from a higher level in terms of how things are organized and tested it out locally.  I'm generally happy with the decisions here, in particular switching the behavior in inspector-search makes sense to me.

---
Please throughout all these patches can you replace ' == ' with ' === ' and ' != ' with ' !== ' just to fit in with most of the files (unless if individual files are using that style explicitly)

::: devtools/client/shared/autocomplete-popup.js:311
(Diff revision 4)
> +
> +  /**
> +   * Find the active element if it belongs in a child document of the autocomplete
> +   * document.
> +   */
> +  _findActiveElement: function () {

I'm not sure about this.  Seems like this._document is the toolbox doc, so document.activeElement might not necessarily going to be inside the popup.

Looks like the intent here is to find anything in this.elements that has the "aria-activedescendant" attribute, so maybe we should just do that directly instead of relying on document.activeElement to find it.

::: devtools/client/shared/autocomplete-popup.js:349
(Diff revision 4)
>     *        The number (index) of the item you want to select in the list.
>     */
>    set selectedIndex(index) {
> -    this._list.selectedIndex = index;
> -    if (this.isOpen && this._list.ensureIndexIsVisible) {
> -      this._list.ensureIndexIsVisible(this._list.selectedIndex);
> +    let previousSelected = this._list.querySelector(".autocomplete-selected");
> +    if (previousSelected) {
> +      previousSelected && previousSelected.classList.remove("autocomplete-selected");

Unneeded `previousSelected &&`
Attachment #8764257 - Flags: review?(bgrinstead)
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/4-5/
Attachment #8764257 - Flags: review?(bgrinstead)
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/4-5/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/5-6/
https://reviewboard.mozilla.org/r/59774/#review58278

> Unintentional console.log?

Removed, thanks!

> Why's this needed?  Please add a comment

I was frequently hit by intermittent failures while testing, so I added this as a workaround. There is an overall race condition issue that occurs when using addTab and trying to focus() an element, but this is being investigated in Bug 1272823, so I am removing the workaround from here. I will do another try push, to check the intermittent failures are not more frequent than they are today.
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/5-6/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/6-7/
https://reviewboard.mozilla.org/r/59772/#review58162

Done in autocomplete.js, inspector-search.js, HTMLTooltip.js and some of the test files. 
Should we enforce === & !== via eslint ?

> I'm not sure about this.  Seems like this._document is the toolbox doc, so document.activeElement might not necessarily going to be inside the popup.
> 
> Looks like the intent here is to find anything in this.elements that has the "aria-activedescendant" attribute, so maybe we should just do that directly instead of relying on document.activeElement to find it.

findActiveElement is supposed to return the focused input for which we are displaying the autocomplete. It should never be in the autocomplete popup, but it should be in one of the subdocuments of the toolbox document.

On this input, we add the id of the currently selected autocomplete-item as the "aria-activedescendant" attribute. 
It seems to me this works as expected. However, there might be an easier solution. We could keep a reference to the anchor passed to AutocompletePopup::show(). I think in all cases, this anchor is the autocompleted input, and will be the "active element" we are looking for.
(In reply to Julian Descottes [:jdescottes] from comment #61)
> 
> It seems to me this works as expected. However, there might be an easier
> solution. We could keep a reference to the anchor passed to
> AutocompletePopup::show(). I think in all cases, this anchor is the
> autocompleted input, and will be the "active element" we are looking for.

Spoke too fast, this won't work for the sourceeditor autocomplete which uses the codemirror cursor element as an anchor. 
The current implementation isets the aria-activedescendant on the codemirror iframe.
The implementation you reviewed sets the aria-activedescendant on the codemirror textarea (which seems more correct IMO).
Yura, we have a setup in devtools where we have an autocomplete list that pops up below various input fields (in the inspector search, css property editing, webconsole input, etc).  We are attempting to set the "aria-activedescendant" attribute on the input field to match the ID of the selected autocomplete item (see Comment 61).  However, the autocomplete items are in a different frame than the input field so that the 'popup' can extend outside of the individual tool's frame (it's attached in the top-level Toolbox document).  So, will this attribute work properly across frames?  If not, is there something else we could do to make this control more accessible?
Flags: needinfo?(yzenevich)
(In reply to Julian Descottes [:jdescottes] from comment #61)
> On this input, we add the id of the currently selected autocomplete-item as
> the "aria-activedescendant" attribute. 
> It seems to me this works as expected. However, there might be an easier
> solution. We could keep a reference to the anchor passed to
> AutocompletePopup::show(). I think in all cases, this anchor is the
> autocompleted input, and will be the "active element" we are looking for.

I think keeping a reference to the active input for the autocomplete would be more straightforward here (and prevent any sort of weird situation where some other element gets the attribute even if that doesn't seem possible).  Unless if there's a reason not to, I'd say do that.
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
(In reply to Brian Grinstead [:bgrins] from comment #64)
> (In reply to Julian Descottes [:jdescottes] from comment #61)
> > On this input, we add the id of the currently selected autocomplete-item as
> > the "aria-activedescendant" attribute. 
> > It seems to me this works as expected. However, there might be an easier
> > solution. We could keep a reference to the anchor passed to
> > AutocompletePopup::show(). I think in all cases, this anchor is the
> > autocompleted input, and will be the "active element" we are looking for.
> 
> I think keeping a reference to the active input for the autocomplete would
> be more straightforward here (and prevent any sort of weird situation where
> some other element gets the attribute even if that doesn't seem possible). 
> Unless if there's a reason not to, I'd say do that.

As I mentioned in comment#62, one issue is that we don't always know the input for which the autocomplete popup is displayed.

For the the sourceeditor autocomplete, the only element provided to the autocomplete popup is the "caret" code mirror element, whereas the active input is actually a textarea.
In order for the autocomplete popup to keep a reference on the active input, we need to modify the AutocompletePopup::openPopup signature to accept an optional "input" parameter that has to be specified when the anchor is not the input. Or did you have something else in mind?
Flags: needinfo?(bgrinstead)
Marco, would you have a chance to also look at the question in Comment 63?
Flags: needinfo?(bgrinstead) → needinfo?(mzehe)
(In reply to Brian Grinstead [:bgrins] from comment #63)
> Yura, we have a setup in devtools where we have an autocomplete list that
> pops up below various input fields (in the inspector search, css property
> editing, webconsole input, etc).  We are attempting to set the
> "aria-activedescendant" attribute on the input field to match the ID of the
> selected autocomplete item (see Comment 61).  However, the autocomplete
> items are in a different frame than the input field so that the 'popup' can
> extend outside of the individual tool's frame (it's attached in the
> top-level Toolbox document).  So, will this attribute work properly across
> frames?  If not, is there something else we could do to make this control
> more accessible?
I've actually never seen this being done, but it should, in theory, be possible. I don't think we have a rule saying that the target ID can't be in a separate sub document. Was there a particular test you tried that didn't work? Or is this more of a general question?
Flags: needinfo?(mzehe)
Remove the feature while waiting for feedback on how to properly do accessibility
when the input and the suggestions live in different frames.

Review commit: https://reviewboard.mozilla.org/r/62840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62840/
Attachment #8768799 - Flags: review?(bgrinstead)
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/5-6/
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/6-7/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/7-8/
Brian: As discussed, I added a patch removing the aria active descendant handling from the autocomplete. Let me know if you'd prefer it to be squashed with part 9.
(In reply to Marco Zehe (:MarcoZ) from comment #67)
> I've actually never seen this being done, but it should, in theory, be
> possible. I don't think we have a rule saying that the target ID can't be in
> a separate sub document.

To be clear, the target ID is not in a sub document, but in a document off of a higher level doc.  So the chain looks something like:

- toolbox doc
-- autocomplete list doc (contains the target)
-- inspector doc
--- markup view doc (contains the element with the aria-activedescendant attribute)


> Was there a particular test you tried that didn't
> work? Or is this more of a general question?

I want to make sure that we don't break the accessibility feature when landing this change.  When testing with VoiceOver I don't get any hints even without the patches applied, so I'm guessing that's a limitation of the tool
(In reply to Brian Grinstead [:bgrins] from comment #73)
> To be clear, the target ID is not in a sub document, but in a document off
> of a higher level doc.  So the chain looks something like:
> 
> - toolbox doc
> -- autocomplete list doc (contains the target)
> -- inspector doc
> --- markup view doc (contains the element with the aria-activedescendant
> attribute)
No, we definitely don't have this covered in our mochitests for aria-activedescendant. So I actually don't know what will happen. The best would be to get a try-server build that has this aria-activedescendant processing in place so I can test it in Windows, because:

> When testing with VoiceOver I don't get any hints even
> without the patches applied, so I'm guessing that's a limitation of the tool
More likely a limitation in our implementation of the Apple accessibility framework. It is incomplete, whereas the accessibility implementations on Windows and Linux are complete. So the most reliable way to test this live is in Windows or Linux with the NVDA or Orca screen readers respectively. And again, I am happy to provide testing on Windows if I get a try build of this patch series.
(In reply to Marco Zehe (:MarcoZ) from comment #74)
> (In reply to Brian Grinstead [:bgrins] from comment #73)
> > To be clear, the target ID is not in a sub document, but in a document off
> > of a higher level doc.  So the chain looks something like:
> > 
> > - toolbox doc
> > -- autocomplete list doc (contains the target)
> > -- inspector doc
> > --- markup view doc (contains the element with the aria-activedescendant
> > attribute)
> No, we definitely don't have this covered in our mochitests for
> aria-activedescendant. So I actually don't know what will happen. The best
> would be to get a try-server build that has this aria-activedescendant
> processing in place so I can test it in Windows, because:
> 
> > When testing with VoiceOver I don't get any hints even
> > without the patches applied, so I'm guessing that's a limitation of the tool
> More likely a limitation in our implementation of the Apple accessibility
> framework. It is incomplete, whereas the accessibility implementations on
> Windows and Linux are complete. So the most reliable way to test this live
> is in Windows or Linux with the NVDA or Orca screen readers respectively.
> And again, I am happy to provide testing on Windows if I get a try build of
> this patch series.

You can use https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab9664aa50be it is a few days old but it has the implementation discussed here.
OK, a quick test doesn't give me any focus events for the autocomplete items, I only get the value change event when the first item is put in the textbox (for example, when typing in ta, and the only item is "table". I do see the autocomplete item in the accessible hierarchy in an adjacent document as you described, but it appears that the ID reference does not work across boundaries of documents/frames.
(In reply to Marco Zehe (:MarcoZ) from comment #76)
> OK, a quick test doesn't give me any focus events for the autocomplete
> items, I only get the value change event when the first item is put in the
> textbox (for example, when typing in ta, and the only item is "table". I do
> see the autocomplete item in the accessible hierarchy in an adjacent
> document as you described, but it appears that the ID reference does not
> work across boundaries of documents/frames.

Thanks for checking.  And I imagine getting the ID reference across frames would be hard to implement since there could be more than one element with the same ID in different frames.  Can you think of anything that would work for us in this situation?

We have the popup as a subframe of the tooltip frame so that we can maximize the space of the popup.  Putting it in the same frame would make the space available for the popup tiny in some cases - like in the case of the split webconsole where the user can control how tall it is.
(In reply to Brian Grinstead [:bgrins] from comment #77)
> Thanks for checking.  And I imagine getting the ID reference across frames
> would be hard to implement since there could be more than one element with
> the same ID in different frames.  Can you think of anything that would work
> for us in this situation?
I was just thinking that maybe putting aria-owns on the search field which then points to the ID of the ul that contains the autocomplete items could help. That alters the accessible tree (nothing else) and would therefore force the IDs into the same logical document sub tree inside the accessibility layer. So just add the aria-owns=... to where you also put aria-activedescendant, but poit it at the parent ul element, not the individual autocomplete items, since you want all of them in there.
(In reply to Marco Zehe (:MarcoZ) from comment #78)
> (In reply to Brian Grinstead [:bgrins] from comment #77)
> > Thanks for checking.  And I imagine getting the ID reference across frames
> > would be hard to implement since there could be more than one element with
> > the same ID in different frames.  Can you think of anything that would work
> > for us in this situation?
> I was just thinking that maybe putting aria-owns on the search field which
> then points to the ID of the ul that contains the autocomplete items could
> help. That alters the accessible tree (nothing else) and would therefore
> force the IDs into the same logical document sub tree inside the
> accessibility layer. So just add the aria-owns=... to where you also put
> aria-activedescendant, but poit it at the parent ul element, not the
> individual autocomplete items, since you want all of them in there.

Thanks for the suggestion, will try that. 

The id set in aria-owns will still be for an element in another document than the input. In the aria specifications, it looks like all ID references should be in the same document ("Reference to the ID of another element in the same document" [1]). I was thinking we might have to clone the selected element and insert it in the input's document.

[1] https://www.w3.org/TR/wai-aria/states_and_properties#propcharacteristic_value_header
You might absolutely be right, so far I only know of IDs in the same document, never had a situation like this one.
(In reply to Julian Descottes [:jdescottes] from comment #79)
> (In reply to Marco Zehe (:MarcoZ) from comment #78)
> > (In reply to Brian Grinstead [:bgrins] from comment #77)
> > > Thanks for checking.  And I imagine getting the ID reference across frames
> > > would be hard to implement since there could be more than one element with
> > > the same ID in different frames.  Can you think of anything that would work
> > > for us in this situation?
> > I was just thinking that maybe putting aria-owns on the search field which
> > then points to the ID of the ul that contains the autocomplete items could
> > help. That alters the accessible tree (nothing else) and would therefore
> > force the IDs into the same logical document sub tree inside the
> > accessibility layer. So just add the aria-owns=... to where you also put
> > aria-activedescendant, but poit it at the parent ul element, not the
> > individual autocomplete items, since you want all of them in there.
> 
> Thanks for the suggestion, will try that. 
> 
> The id set in aria-owns will still be for an element in another document
> than the input. In the aria specifications, it looks like all ID references
> should be in the same document ("Reference to the ID of another element in
> the same document" [1]). I was thinking we might have to clone the selected
> element and insert it in the input's document.
> 
> [1]
> https://www.w3.org/TR/wai-aria/
> states_and_properties#propcharacteristic_value_header

I'm pretty certain id's from other documents won't be referenced. Had tried that in B2G
Flags: needinfo?(yzenevich)
Blocks: 1285591
Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59770/diff/6-7/
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59772/diff/7-8/
Comment on attachment 8764258 [details]
Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59774/diff/8-9/
Comment on attachment 8768799 [details]
Bug 1266456 - part11: a11y, stop handling aria active-descendant in autocomplete;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62840/diff/1-2/
Brian: Just rebased and pushed to try again https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d5872e0df22
As discussed during stand up, I will fold part11 into part9 before landing. Just leaving them separate for you to test&review.
Thanks Yura and Marco for having a look at the issue! 

Just wanted to do a quick summary of the plan here for accessibility:
We want to land the HTML based autocompletes here, to get enough testing time before the next branching point.
Let's work on re-enabling accessibility features in parallel in Bug 1285591.
Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

https://reviewboard.mozilla.org/r/59772/#review60050
Attachment #8764257 - Flags: review?(bgrinstead) → review+
Comment on attachment 8768799 [details]
Bug 1266456 - part11: a11y, stop handling aria active-descendant in autocomplete;

https://reviewboard.mozilla.org/r/62840/#review60052
Attachment #8768799 - Flags: review?(bgrinstead) → review+
Thanks for the reviews, folded part 11 into part 9 and pushed to try again : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c50b3a8c43c9
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/972c11c59a0c
part8: move HTML search autocomplete css to tooltips.css;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/548c72c399ec
part9: use HTMLTooltip for autocomplete-popup;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/1de829f2f1f0
part10: fix tests for autocomplete-popup HTMLTooltip migration;r=bgrins
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1286523
No new issues found while verifying this fix using latest Nightly 50.0a1, under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Blocks: 1290222
No longer blocks: 1294038
Depends on: 1294038
Target Milestone: --- → Firefox 50
Depends on: 1327121
Depends on: 1327153
Depends on: 1327378
Depends on: 1327994
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.