1266456 - Create a HTML replacement for the autocomplete suggestion popup

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part1: HTMLTooltip cleanup test & jsdoc for synchronous setContent;

Review commit: https://reviewboard.mozilla.org/r/59756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59756/

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part2: HTMLTooltip setContent() use object as 2nd arg;

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/

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part3: HTMLTooltip show() clear timeout to avoid window leaks;

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/

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part4: HTMLTooltip support XUL docs using page instead of window;

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/

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part5: HTMLTooltip setContent() support "auto" width parameter;

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/

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part6: HTMLTooltip show() now accepts x,y offsets;

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/

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part7: tooltips.css remove unused event details tooltip rule;

Review commit: https://reviewboard.mozilla.org/r/59768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59768/

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

Review commit: https://reviewboard.mozilla.org/r/59770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59770/

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

Review commit: https://reviewboard.mozilla.org/r/59772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59772/

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part10: fix tests for autocomplete-popup HTMLTooltip migration;

Review commit: https://reviewboard.mozilla.org/r/59774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59774/

Comment 11

[Alexandre Poirot [:ochameau]]

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 12

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8764249 [details]
Bug 1266456 - part1: HTMLTooltip cleanup test & jsdoc for synchronous setContent;

https://reviewboard.mozilla.org/r/59756/#review57256

Comment 13

[Jarda Snajdr [:jsnajdr]]

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.

Comment 14

[Julian Descottes [:jdescottes]]

(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 15

[Julian Descottes [:jdescottes]]

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 16

[Julian Descottes [:jdescottes]]

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 17

[Julian Descottes [:jdescottes]]

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 18

[Julian Descottes [:jdescottes]]

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 19

[Julian Descottes [:jdescottes]]

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 20

[Julian Descottes [:jdescottes]]

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 21

[Julian Descottes [:jdescottes]]

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 22

[Julian Descottes [:jdescottes]]

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 23

[Julian Descottes [:jdescottes]]

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 24

[Julian Descottes [:jdescottes]]

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/

Comment 25

[Julian Descottes [:jdescottes]]

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 26

[Brian Grinstead [:bgrins]]

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?

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8764254 [details]
Bug 1266456 - part6: HTMLTooltip show() now accepts x,y offsets;

https://reviewboard.mozilla.org/r/59766/#review57120

Comment 28

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8764250 [details]
Bug 1266456 - part2: HTMLTooltip setContent() use object as 2nd arg;

https://reviewboard.mozilla.org/r/59758/#review57378

Comment 29

[Jarda Snajdr [:jsnajdr]]

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+.

Comment 30

[Julian Descottes [:jdescottes]]

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 31

[Jarda Snajdr [:jsnajdr]]

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.

Comment 32

[Julian Descottes [:jdescottes]]

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 33

[Julian Descottes [:jdescottes]]

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 34

[Julian Descottes [:jdescottes]]

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 35

[Julian Descottes [:jdescottes]]

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 36

[Julian Descottes [:jdescottes]]

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 37

[Julian Descottes [:jdescottes]]

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 38

[Julian Descottes [:jdescottes]]

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 39

[Julian Descottes [:jdescottes]]

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/

Comment 40

[Julian Descottes [:jdescottes]]

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!

Comment 41

[Julian Descottes [:jdescottes]]

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.

Comment 42

[Julian Descottes [:jdescottes]]

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

Comment 43

[Julian Descottes [:jdescottes]]

try is green, landing parts 1-7.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
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 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a4f5f3b73e2
https://hg.mozilla.org/mozilla-central/rev/682957e33397
https://hg.mozilla.org/mozilla-central/rev/4f42717b9103
https://hg.mozilla.org/mozilla-central/rev/b3e5f1ec1af3
https://hg.mozilla.org/mozilla-central/rev/56aa4e3e6287
https://hg.mozilla.org/mozilla-central/rev/88e3339aaec1
https://hg.mozilla.org/mozilla-central/rev/9688cac5afbe

Comment 46

[Julian Descottes [:jdescottes]]

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/

Comment 47

[Julian Descottes [:jdescottes]]

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 48

[Julian Descottes [:jdescottes]]

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/

Comment 49

[Julian Descottes [:jdescottes]]

Had a green try yesterday (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a01ba974a74e5fb77f763c431d20e79291facd8),  but cleaned up the last commits for review so new push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6a578de976

Comment 50

[Julian Descottes [:jdescottes]]

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/

Comment 51

[Julian Descottes [:jdescottes]]

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 52

[Brian Grinstead [:bgrins]]

Comment on attachment 8764256 [details]
Bug 1266456 - part8: move HTML search autocomplete css to tooltips.css;

https://reviewboard.mozilla.org/r/59770/#review57996

Comment 53

[Brian Grinstead [:bgrins]]

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

Comment 54

[Brian Grinstead [:bgrins]]

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 &&`

Comment 55

[Julian Descottes [:jdescottes]]

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/

Comment 56

[Julian Descottes [:jdescottes]]

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 57

[Julian Descottes [:jdescottes]]

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/

Comment 58

[Julian Descottes [:jdescottes]]

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 59

[Julian Descottes [:jdescottes]]

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 60

[Julian Descottes [:jdescottes]]

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/

Comment 61

[Julian Descottes [:jdescottes]]

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.

Comment 62

[Julian Descottes [:jdescottes]]

(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).

Comment 63

[Brian Grinstead [:bgrins]]

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?

Comment 64

[Brian Grinstead [:bgrins]]

(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.

Comment 65

[Julian Descottes [:jdescottes]]

(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?

Comment 66

[Brian Grinstead [:bgrins]]

Marco, would you have a chance to also look at the question in Comment 63?

Comment 67

[Marco Zehe (:MarcoZ)]

(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?

Comment 68

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266456 - part11: a11y, stop handling aria active-descendant in autocomplete;

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/

Comment 69

[Julian Descottes [:jdescottes]]

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 70

[Julian Descottes [:jdescottes]]

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 71

[Julian Descottes [:jdescottes]]

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/

Comment 72

[Julian Descottes [:jdescottes]]

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.

Comment 73

[Brian Grinstead [:bgrins]]

(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

Comment 74

[Marco Zehe (:MarcoZ)]

(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.

Comment 75

[Julian Descottes [:jdescottes]]

(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.

Comment 76

[Marco Zehe (:MarcoZ)]

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.

Comment 77

[Brian Grinstead [:bgrins]]

(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.

Comment 78

[Marco Zehe (:MarcoZ)]

(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.

Comment 79

[Julian Descottes [:jdescottes]]

(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

Comment 80

[Marco Zehe (:MarcoZ)]

You might absolutely be right, so far I only know of IDs in the same document, never had a situation like this one.

Comment 81

[Yura Zenevich [:yzen] (please use "needinfo")]

(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

Comment 82

[Julian Descottes [:jdescottes]]

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 83

[Julian Descottes [:jdescottes]]

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 84

[Julian Descottes [:jdescottes]]

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 85

[Julian Descottes [:jdescottes]]

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/

Comment 86

[Julian Descottes [:jdescottes]]

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.

Comment 87

[Julian Descottes [:jdescottes]]

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 88

[Brian Grinstead [:bgrins]]

Comment on attachment 8764257 [details]
Bug 1266456 - part9: use HTMLTooltip for autocomplete-popup;

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

Comment 89

[Brian Grinstead [:bgrins]]

Comment on attachment 8768799 [details]
Bug 1266456 - part11: a11y, stop handling aria active-descendant in autocomplete;

https://reviewboard.mozilla.org/r/62840/#review60052

Comment 90

[Julian Descottes [:jdescottes]]

Thanks for the reviews, folded part 11 into part 9 and pushed to try again : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c50b3a8c43c9

Comment 91

[Pulsebot]

Pushed by [e-mail]:
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

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/972c11c59a0c
https://hg.mozilla.org/mozilla-central/rev/548c72c399ec
https://hg.mozilla.org/mozilla-central/rev/1de829f2f1f0

Comment 93

[Ada [:adalucinet]]

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.