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
Created attachment 422052 [details] Patch
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.
Created attachment 422189 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
(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.
(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.
<rdar://problem/75255277>
Created attachment 422808 [details] Rebased patch
(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.
(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.
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.
*** This bug has been marked as a duplicate of bug 241655 ***