Closed Bug 1488012 Opened 4 years ago Closed 4 years ago

Update frame picker button to use Photon styling


(DevTools :: General, enhancement, P3)



(firefox64 fixed)

Firefox 64
Tracking Status
firefox64 --- fixed


(Reporter: fvsch, Assigned: fvsch)




(1 file, 2 obsolete files)

Design review ongoing for an updated "Select an iframe as the currently targetted document" button:

Opening this bug for tracking the implementation.
I managed to mark the button as “checked” if the currently selected frame is not a top-level document.

But that `isChecked` function is not called when selecting a new frame, it seems to wait on the next re-render of the toolbar. I guess the RDM button has the same issue and manages it in some way, I’ll have to look more.
Haven't found a way to update the “checked” status earlier yet. Any suggestion is welcome.

Ideally, instead of adding a `checked` class to the button when its menu is shown, I’d like to add `aria-expanded="true"`. This would enable us to have two different visual states:
- checked, using the blue color
- expanded, using the hover background-color

I’ve opened a UX issue to propose this behavior:
(In reply to Florens Verschelde from comment #3)
> Haven't found a way to update the “checked” status earlier yet. Any
> suggestion is welcome.

Looks like there are many ways to trigger an update here, hard to know which one is the best. The first thing is that this update should only be done after the selected frame has been updated. This is an async process which ends in "_updateFrames". So from this method we could trigger an update by calling `this.frameButton.emit("updatechecked");`, but as it happens we also have a updateFrameButton method which updates properties of the button one by one. I feel like it would almost make sense to set isChecked there rather than passing a method ... it's not like the buttons updates automagically anyway, so why not set the property directly.

Also in the patch attached, I removed two spots were we set "isChecked" when the menu opens/collapses. Not sure how you want to handle that, so just pointing it out.
Assignee: nobody → florens
Severity: normal → enhancement
Ever confirmed: true
Priority: -- → P3
Florens, your patch is already up for review, but maybe you want to have a go at fixing the update before we do the review?
Flags: needinfo?(florens)
Sorry, I reattached your own patch by mistake :)
Attachment #9006058 - Attachment is obsolete: true
Yeah I’d like to improve the patch before that. I had to mark it for review on Phabricator or moz-phab wouldn't let me push it. Maybe arc is more flexible.

I’ll also wait for Bug 1476549 to land and rebase on top of it.
Depends on: 1476549
Flags: needinfo?(florens)
Patch updated on
I removed the old `toolbox._framesButtonChecked` implementation which was always set to false and seems to have never been used.

I'm not sure if the "is the selected frame the parent document" test works perfectly. I'm getting good results in my tests on (4 iframes on the same origin), but in the Browser Toolbox it looks like all XUL documents or maybe all dummy.xul documents end up testing as "top level".

Also if we're adding "show a selected/active style when a subdocument is selected" as a feature, perhaps we should write a test for it?
In my tests, it turns out that:

- for Web content, all subdocuments have a parentID
- for chrome content, some do, some don't

I tried a few things, and the least ugly was to check if the first frame in the frameMap is "chrome://browser/content/browser.xul".
Attachment #9006060 - Attachment is obsolete: true
Attachment #9005847 - Attachment description: Bug 1488012 - Update devtools frame picker button; r=gl → Bug 1488012 - Update devtools frame picker button; r?jdescottes
Pushed by
Update devtools frame picker button; r=jdescottes
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.