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
11 months ago

People

(Reporter: bgrins, Assigned: julian.descottes)

Tracking

({regression})

Trunk
Firefox 42

Firefox Tracking Flags

(firefox41 unaffected, firefox42 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
This doesn't happen for me on dev edition right now, so looks like something that's regressed in 42
(Reporter)

Comment 2

4 years ago
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
(Reporter)

Updated

4 years ago
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?
(Reporter)

Comment 4

4 years ago
(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
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Quick tests on Firefox 39 and 41 show the same issue.
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 11

4 years ago
> 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)
(Assignee)

Updated

4 years ago
No longer depends on: 1190944
(Reporter)

Updated

4 years ago
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+
(Reporter)

Comment 14

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(Assignee)

Comment 17

4 years ago
Thanks for the review and commit ! Glad I could help.
Duplicate of this bug: 1170013
Duplicate of this bug: 1141148

Updated

4 years ago
See Also: → 1192172

Comment 20

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1192172
See Also: 1192172

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.