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

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: bgrins, Assigned: julian.descottes)

Tracking

({regression})

Trunk
Firefox 42
Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected, firefox42 fixed)

Details

Attachments

(1 attachment)

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
This doesn't happen for me on dev edition right now, so looks like something that's regressed in 42
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
Summary: Autocomplete → Autocomplete popup doesn't show up when pressing down in an empty property value field
A wild guess, but maybe bug 1190944 is related?
(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
Depends on: 1190944
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.
Quick tests on Firefox 39 and 41 show the same issue.
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
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 ?
Flags: needinfo?(bgrinstead)
(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?
(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.
> 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)
Assignee: nobody → julian.descottes
Flags: needinfo?(bgrinstead)
No longer depends on: 1190944
Status: NEW → ASSIGNED
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!
Attachment #8643968 - Flags: feedback+
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.
Attachment #8643968 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/a8dbc50c600d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Thanks for the review and commit ! Glad I could help.
Duplicate of this bug: 1170013
Duplicate of this bug: 1141148
See Also: → 1192172
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.
Depends on: 1192172
See Also: 1192172
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.