Closed Bug 1392873 Opened 2 years ago Closed 2 years ago

all-tabs-menu doesn't work when Inspector is in separate window

Categories

(DevTools :: Inspector, defect)

54 Branch
defect
Not set

Tracking

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: euthanasia_waltz, Assigned: rickychien)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
1. Open Inspector
2. Show it in separate window
3. Click all-tabs-menu(▼)

AR:
Nothing happened.

ER:
All tabs menu(Rules/Computed/Animations/Fonts) opened.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: 55 Branch → 54 Branch
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ebf65a1af072e0c4d258257477812dc66c8b635d&tochange=0fc0b762b40a72ffdb519167dd5b249a3ea629b2

Regressed by: 0fc0b762b40a	Ricky Chien — Bug 1344155 - Remove toolbox in netmonitor r=Honza


An error is shown in Browser Console when click the dropdown marker of inspector window:

TypeError: popupset is null[Learn More]  menu.js:72:7


:rickychien,
Your patch seems to cause the regression. Could you look this?
Blocks: 1344155
Flags: needinfo?(rchien)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
The root cause is because that the popup's document could be changed when detaching devtools panel to a separate window. I'm going to introduce menuDocument props to address this issue.
Comment on attachment 8900126 [details]
Bug 1392873 - Fix missing all-tabs-menu when detaching devtools to a separate window

https://reviewboard.mozilla.org/r/171520/#review177418

Thanks for the patch Ricky!

Honza

::: devtools/client/shared/components/tabs/tabbar.js:201
(Diff revision 1)
>      // https://bugzilla.mozilla.org/show_bug.cgi?id=1274551
>      let rect = target.getBoundingClientRect();
>      let screenX = target.ownerDocument.defaultView.mozInnerScreenX;
>      let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
>      menu.popup(rect.left + screenX, rect.bottom + screenY,
> -      { doc: window.parent.document });
> +      { doc: this.props.menuDocument });

What about having fallback to `window.parent.document` in case the `menuDocument` isn't set?

Honza
Comment on attachment 8900126 [details]
Bug 1392873 - Fix missing all-tabs-menu when detaching devtools to a separate window

https://reviewboard.mozilla.org/r/171520/#review177892

Looks good, thanks!

Honza
Attachment #8900126 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ec1350d5cc3
Fix missing all-tabs-menu when detaching devtools to a separate window r=Honza
https://hg.mozilla.org/mozilla-central/rev/5ec1350d5cc3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I can reproduce this issue in Nightly following the str from comment 0 with 57.0a1 (2017-08-22) (64-bit) in 64-bit Linux.

I can verify that this bug is fixed in latest Nightly 57.0a1

Build ID 	20170826100418
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170823]
I have successfully reproduced the bug in firefox Nightly 57.0a1 (2017-08-22) (64-bit) with windows 10 (64 bit) 

Verified as fixed with latest nightly 57.0a1 (2017-08-26) (64-bit)


Build ID :	20170826100418
User Agent : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170823]
This bug is verified in both linux(comment 10) and windows(comment 11). So, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Please request Beta approval on this when you get a chance.
Flags: needinfo?(rchien)
Comment on attachment 8900126 [details]
Bug 1392873 - Fix missing all-tabs-menu when detaching devtools to a separate window

Approval Request Comment
[Feature/Bug causing the regression]: regression from bug 1344155
[User impact if declined]: usability impact
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: minor
[Why is the change risky/not risky?]: only happen when detaching Devtools panel.
[String changes made/needed]: none
Flags: needinfo?(rchien)
Attachment #8900126 - Flags: approval-mozilla-beta?
Comment on attachment 8900126 [details]
Bug 1392873 - Fix missing all-tabs-menu when detaching devtools to a separate window

Fix a regression and was verified. Beta56+.
Attachment #8900126 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.