Bug 222644 - Web Inspector: Allow forcing pseudo class :focus-within
Summary: Web Inspector: Allow forcing pseudo class :focus-within
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: InRadar
Depends on: 222990
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-03 01:24 PST by Manuel Rego Casasnovas
Modified: 2021-03-16 03:07 PDT (History)
17 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2021-03-03 01:30 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (12.21 KB, patch)
2021-03-04 00:43 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Rebased patch (6.18 KB, patch)
2021-03-10 01:29 PST, Manuel Rego Casasnovas
rego: review?
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2021-03-03 01:24:44 PST
Similar to what's already allowed for :focus, it'd be nice to have the chance to force :focus-within from the web inspector.visible
Comment 1 Manuel Rego Casasnovas 2021-03-03 01:30:52 PST
Created attachment 422052 [details]
Patch
Comment 2 Devin Rousso 2021-03-03 16:06:38 PST
Comment on attachment 422052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422052&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:707
> +WI.CSSManager.ForceablePseudoClasses = ["active", "focus", "focus-within", "hover", "visited"];

You should also add this to the inspector protocol (specifically the enum in `forcePseudoState` in `Source/JavaScriptCore/inspector/protocol/CSS.json`).

Also, what does this look like in the Web Inspector frontend UI?  Right now there's enough space for all four existing checkboxes (Active, Focus, Hover, Visited), but I don't know that there is for a fifth.
Comment 3 Manuel Rego Casasnovas 2021-03-04 00:43:56 PST
Created attachment 422189 [details]
Patch
Comment 4 EWS Watchlist 2021-03-04 00:45:07 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 5 Manuel Rego Casasnovas 2021-03-04 00:47:52 PST
(In reply to Devin Rousso from comment #2)
> Comment on attachment 422052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422052&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:707
> > +WI.CSSManager.ForceablePseudoClasses = ["active", "focus", "focus-within", "hover", "visited"];
> 
> You should also add this to the inspector protocol (specifically the enum in
> `forcePseudoState` in `Source/JavaScriptCore/inspector/protocol/CSS.json`).

Done.

> Also, what does this look like in the Web Inspector frontend UI?  Right now
> there's enough space for all four existing checkboxes (Active, Focus, Hover,
> Visited), but I don't know that there is for a fifth.

Checking this in different cases, sometimes it doesn't fit. So yeah more changes would be needed to add this. :-(

I see that if "exclusive-presentation" is used, it has "flex-flow: row nowrap", so the row doesn't wrap and it doesn't fit depending on the width.

When it doesn't have "exclusive-presentation" and depending on the width you might end up with 3 rows, but things work fine in that case.

Dunno if you think this is interesting to add or not, and some extra work should be done here.
Other browsers have this, but maybe we don't want to add it.
Note that in the future we might want to add :focus-visible too, once it's shipped by default.
Comment 6 Patrick Angle 2021-03-09 13:31:38 PST
(In reply to Manuel Rego Casasnovas from comment #5)
> (In reply to Devin Rousso from comment #2)
> > Also, what does this look like in the Web Inspector frontend UI?  Right now
> > there's enough space for all four existing checkboxes (Active, Focus, Hover,
> > Visited), but I don't know that there is for a fifth.
> I see that if "exclusive-presentation" is used, it has "flex-flow: row
> nowrap", so the row doesn't wrap and it doesn't fit depending on the width.
> 
> When it doesn't have "exclusive-presentation" and depending on the width you
> might end up with 3 rows, but things work fine in that case.
When I added the mechanism for showing the `Styles` sidebar next to other detail sidebars when there is enough space, I added this rule to make sure that the header for the Styles panel matches the height of the navigation bar for the rest of the sidebar items.

Also worth noting is that `WI.GeneralStyleDetailsSidebarPanel.minimumWidth` has to calculate the width of these items to determine the minimum width of the panel, so we should make sure that gets updated with whatever we decide to do here.
Comment 7 Radar WebKit Bug Importer 2021-03-10 01:25:15 PST
<rdar://problem/75255277>
Comment 8 Manuel Rego Casasnovas 2021-03-10 01:29:07 PST
Created attachment 422808 [details]
Rebased patch
Comment 9 Manuel Rego Casasnovas 2021-03-10 01:41:08 PST
(In reply to Patrick Angle from comment #6)
> (In reply to Manuel Rego Casasnovas from comment #5)
> > (In reply to Devin Rousso from comment #2)
> > > Also, what does this look like in the Web Inspector frontend UI?  Right now
> > > there's enough space for all four existing checkboxes (Active, Focus, Hover,
> > > Visited), but I don't know that there is for a fifth.
> > I see that if "exclusive-presentation" is used, it has "flex-flow: row
> > nowrap", so the row doesn't wrap and it doesn't fit depending on the width.
> > 
> > When it doesn't have "exclusive-presentation" and depending on the width you
> > might end up with 3 rows, but things work fine in that case.
> When I added the mechanism for showing the `Styles` sidebar next to other
> detail sidebars when there is enough space, I added this rule to make sure
> that the header for the Styles panel matches the height of the navigation
> bar for the rest of the sidebar items.
> 
> Also worth noting is that `WI.GeneralStyleDetailsSidebarPanel.minimumWidth`
> has to calculate the width of these items to determine the minimum width of
> the panel, so we should make sure that gets updated with whatever we decide
> to do here.

GeneralStyleDetailsSidebarPanel.minimumWidth already takes into account the size of the new pseudo-class. When I resize just that panel, the minimum size is enforced and :focus-within is consiered.

The problem is when the panel appears for the first time, or when we resize the main window and the "exclusive" sidebar disappears and appears.

Trying to fix that I modified WI.SingleSidebar.minimumWidth() to add:
        if (this.selectedSidebarPanel)
            minimumWidth = Math.max(minimumWidth, this.selectedSidebarPanel.minimumWidth);

However that didn't solve the issue yet, for 2 reasons:
* GeneralStyleDetailsSidebarPanel.exclusive is "false" when this is called, so GeneralStyleDetailsSidebarPanel.minimumWidth doesn't take into account the pseudo-classes.
* this.selectedSidebarPanel is not always set when it's called, for example when we're increasing the windows size and the "exclusive" sidebar appears.

Anyway I have never touched the inspector code before, and probably if we're going to add :focus-within and :focus-visible (in the future) to the list of pseudo-classes, some UI changes might be considered too. Dunno.
Comment 10 Patrick Angle 2021-03-10 09:28:25 PST
(In reply to Manuel Rego Casasnovas from comment #9)
> The problem is when the panel appears for the first time, or when we resize
> the main window and the "exclusive" sidebar disappears and appears.
> 
> Trying to fix that I modified WI.SingleSidebar.minimumWidth() to add:
>         if (this.selectedSidebarPanel)
>             minimumWidth = Math.max(minimumWidth,
> this.selectedSidebarPanel.minimumWidth);
> 
> However that didn't solve the issue yet, for 2 reasons:
> * GeneralStyleDetailsSidebarPanel.exclusive is "false" when this is called,
> so GeneralStyleDetailsSidebarPanel.minimumWidth doesn't take into account
> the pseudo-classes.
> * this.selectedSidebarPanel is not always set when it's called, for example
> when we're increasing the windows size and the "exclusive" sidebar appears.

Yeah, I actually opened this exact bug yesterday (bug 222990), which when resolved should make this no longer an issue. Adding a checkbox makes this much more apparent than before, where only a bit of padding was being trimmed off the sides.
Comment 11 Manuel Rego Casasnovas 2021-03-16 03:07:06 PDT
After bug #222990 has been fixed, this patch still introduces an issue when the inspector is opened the first time (at that point it doesn't take into account the size of the checkboxes). Later if you resize anything it works fine.

Though maybe we want to improve the UI before having a list of checkboxes taking so many space.