Closed
Bug 1191093
Opened 9 years ago
Closed 9 years ago
Autocomplete popup doesn't show up when pressing down in an empty property value field
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox41 unaffected, firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox41 | --- | unaffected |
firefox42 | --- | fixed |
People
(Reporter: bgrins, Assigned: julian.descottes)
References
Details
(Keywords: regression)
Attachments
(1 file)
11.77 KB,
patch
|
bgrins
:
review+
gl
:
feedback+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
This doesn't happen for me on dev edition right now, so looks like something that's regressed in 42
status-firefox41:
--- → unaffected
Keywords: regression
Reporter | ||
Comment 2•9 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•9 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•9 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•9 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•9 years ago
|
||
Quick tests on Firefox 39 and 41 show the same issue.
Assignee | ||
Comment 7•9 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•9 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)
Comment 9•9 years ago
|
||
(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•9 years ago
|
||
(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•9 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 | ||
Comment 12•9 years ago
|
||
Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e4c8125794
Attachment #8643968 -
Flags: review?(mratcliffe)
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 13•9 years ago
|
||
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•9 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+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a8dbc50c600d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee | ||
Comment 17•9 years ago
|
||
Thanks for the review and commit ! Glad I could help.
Comment 20•9 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•9 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•