1191093 - Autocomplete popup doesn't show up when pressing down in an empty property value field

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

STR:

Open data:text/html,<style>body{display:block;}</style>
Inspect the body
Highlight and delete "block" in the rule view
Press down

Expected:

Autocomplete popup suggestions show up

Actual: 

-moz-box is selected (first selection) and no popup shows up

Comment 1

[Brian Grinstead [:bgrins]]

This doesn't happen for me on dev edition right now, so looks like something that's regressed in 42

Comment 2

[Brian Grinstead [:bgrins]]

I see these errors in the Browser Console (probably because the element is hidden and it's bindings have gone away):

TypeError: this._list.ensureIndexIsVisible is not a function autocomplete-popup.js:305:5
TypeError: this._panel.hidePopup is not a function autocomplete-popup.js:161:5

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

A wild guess, but maybe bug 1190944 is related?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> A wild guess, but maybe bug 1190944 is related?

Actually, in a quick test I think it might. Marking it as dependent so we can check again when it's resolved

Comment 5

[Julian Descottes]

I tested this after fixing 1190944 and it doesn't seem to be linked to the same issue.

Also, I think the correct STR are :

-Open Firefox
-Open data:text/html,<style>body{display:block;}</style>
-Open devtools (!!! important, don't open before navigating)
-Inspect the body
-Highlight and delete "block" in the rule view
-Press down -> list is shown
-Reload (F5), keep devtools opened
-Inspect the body
-Highlight and delete "block" in the rule view
-Press down -> no suggestion list

If you close and reopen the devtools, the suggestion list will work again.

Comment 6

[Julian Descottes]

Quick tests on Firefox 39 and 41 show the same issue.

Comment 7

[Julian Descottes]

After more investigations, I think this is linked to a conflict between :
- markup-view.js
- rule-view.js

The aucomplete-popup instances used by each view share the same XUL elements (_list, _panel), because they attach it to their common parent XUL document. See autocomplete-popup at L56 "// Reuse the existing popup elements."

But the markup-view is destroyed/instanciated every time we navigate. (inspector-panel#_onMarkupFrameLoad, #_onBeforeNavigate etc ...). When it is destroyed it calls the autocomplete-popup destroy method which removes the _list and _panel (see autocomplete-popup#destroy)

Possible solutions :
- attach their autocomplete popups to different documents
- force each autocomplete-popup instance to create its own DOM
- modify autocomplete-popup to verify _list and _panel are still valid references, otherwise either retrieve them on the document, or recreate them

Comment 8

[Julian Descottes]

Following up on my previous comment, an easy fix here would be to provide the optional id when instanciating the popup from markup-view. Since markup view has an odd lifecycle it's safer to make sure it has its own markup. 

@bgrins : what do you think ?

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes from comment #7)
> After more investigations, I think this is linked to a conflict between :
> - markup-view.js
> - rule-view.js
> 
> The aucomplete-popup instances used by each view share the same XUL elements
> (_list, _panel), because they attach it to their common parent XUL document.
> See autocomplete-popup at L56 "// Reuse the existing popup elements."
> 
> But the markup-view is destroyed/instanciated every time we navigate.
> (inspector-panel#_onMarkupFrameLoad, #_onBeforeNavigate etc ...). When it is
> destroyed it calls the autocomplete-popup destroy method which removes the
> _list and _panel (see autocomplete-popup#destroy)
> 
> Possible solutions :
> - attach their autocomplete popups to different documents
> - force each autocomplete-popup instance to create its own DOM
> - modify autocomplete-popup to verify _list and _panel are still valid
> references, otherwise either retrieve them on the document, or recreate them

I think the third proposed solution would be best since we want to continue sharing the same XUL element and avoid creating new DOM elements when we can for best performance. I would definitely like to see this bug resolved before the Firefox 42 merge on Monday. Julian, were you interested in tackling this?

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes from comment #8)
> Following up on my previous comment, an easy fix here would be to provide
> the optional id when instanciating the popup from markup-view. Since markup
> view has an odd lifecycle it's safer to make sure it has its own markup. 
> 
> @bgrins : what do you think ?

I tested your suggestion on adding a panelId to the options provided to AutocompletePopup, and this does resolve the problem and would be a significant improvement to the current behavior. I would be okay with this easy fix considering the timing to the merge date.

Comment 11

[Julian Descottes]

> Julian, were you interested in tackling this?

Sure, assigning it to me !

> I would be okay with this easy fix considering the timing to the merge date.

Agreed. This + a mochitest seems reasonable for a monday target.

(removing ni: for Brian)

Comment 12

[Julian Descottes]

Attachment: Bug1191093.v1.patch

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e4c8125794

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8643968 [details] [diff] [review]
Bug1191093.v1.patch

Review of attachment 8643968 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Julian! The extra test coverage is great!

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8643968 [details] [diff] [review]
Bug1191093.v1.patch

Review of attachment 8643968 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review, r=me.  Thanks for fixing this.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a8dbc50c600d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8dbc50c600d

Comment 17

[Julian Descottes]

Thanks for the review and commit ! Glad I could help.

Comment 20

[arni2033]

It's sad that issues like this are fixed Only when a Dev experiences them, not when somebody reported STR
But anyway, this bug caused a regression described in 1192172. You're really missing a good beta-tester.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/785195348eb8

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/785195348eb8