Update frame picker button to use Photon styling

RESOLVED FIXED in Firefox 64

Status

enhancement
P3
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: fvsch, Assigned: fvsch)

Tracking

unspecified
Firefox 64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

9 months ago
Design review ongoing for an updated "Select an iframe as the currently targetted document" button:
https://github.com/devtools-html/ux/issues/18

Opening this bug for tracking the implementation.
Assignee

Comment 2

9 months ago
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.
Assignee

Comment 3

9 months ago
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:
https://github.com/devtools-html/ux/issues/25
(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
Status: UNCONFIRMED → ASSIGNED
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
Assignee

Comment 8

9 months ago
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.
Assignee

Updated

8 months ago
Depends on: 1476549
Flags: needinfo?(florens)
Assignee

Comment 9

7 months ago
Patch updated on https://phabricator.services.mozilla.com/D4820
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 https://fvsch.com/styling-buttons/ (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?
Assignee

Comment 10

7 months ago
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".
Assignee

Updated

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

Comment 12

7 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3289b676c65b
Update devtools frame picker button; r=jdescottes

Comment 13

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3289b676c65b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.