Bug 222644 – Web Inspector: Allow forcing pseudo class :focus-within

• Component: Web Inspector
• Status: RESOLVED DUPLICATE

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 422052 [details]
Patch

[Devin Rousso]

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.

[Manuel Rego Casasnovas]

Created attachment 422189 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Manuel Rego Casasnovas]

(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.

[Patrick Angle]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/75255277>

[Manuel Rego Casasnovas]

Created attachment 422808 [details]
Rebased patch

[Manuel Rego Casasnovas]

(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.

[Patrick Angle]

(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.

[Manuel Rego Casasnovas]

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.

[Devin Rousso]

*** This bug has been marked as a duplicate of bug 241655 ***