Bug 67127 – Web Inspector: Implement circular tabbing through the Styles sidebar pane contents

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Currently hitting Tab/Enter on a blank new property in the last editable style cancels editing. If a user has done this in error, they will need to use their mouse to restart the style editing.

Instead, the editor should loop to the beginning end of the pane contents.

[Alexander Pavlov (apavlov)]

Created attachment 105495 [details]
[PATCH] Suggested solution

[Pavel Feldman]

Comment on attachment 105495 [details]
[PATCH] Suggested solution

Although I don't see any flags in the code, I suspect this could be implemented via adding less code. Can you track down the case when you jump from the last new property placeholder? The very moment when focus is about to leave the traversal?

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 105495 [details])
> Although I don't see any flags in the code, I suspect this could be implemented via adding less code.

I will look into shortening the change.

> Can you track down the case when you jump from the last new property placeholder? The very moment when focus is about to leave the traversal?

What do you mean by this? Is there anything special about this case? Just a quick overview for editor navigation outside of currently edited properties (jumping between name-value gives a few more cases):

* source editor (jump from):
  1. ordinary property (may get deleted once the editor gets committed)
  2. blank new property
* target editor (jump to):
  1. selector of the same section
  2. selector of another section
  3. ordinary property of the same section
  4. ordinary property of another section
  5. blank new property of the same section
  6. blank new property of another section

11 combinations in total (1-4 excluded, as it is impossible), of which the new ones are (partly due to element.style not having a selector):
1-6 (Jump from the first element.style property backwards to a new blank property of the last editable section). This is handled by the lines 2035-2045.
2-5 (If there is only element.style and no matched rules, jump from element.style's blank property to itself)
2-6 (Jump from a blank new property of the last section to a blank new property of an empty element.style, or vice versa). These two cases (along with the already handled 2-1 and 2-2) are handled by the lines 2114-2128).

I'll try to come up with something shorter for these cases.

[Alexander Pavlov (apavlov)]

Created attachment 106342 [details]
[PATCH] Simplified weird CSS property name/value field editor tabbing calculations

[Alexander Pavlov (apavlov)]

Created attachment 106568 [details]
[PATCH] Extracted a hard part of the calculations into StylePropertiesSection (asked by [e-mail])

[Pavel Feldman]

Comment on attachment 106568 [details]
[PATCH] Extracted a hard part of the calculations into StylePropertiesSection (asked by [e-mail])

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:941
> +            var candidateSection = this;

I'd rather introduce Section::firstSibling - it can be fetched via parent element momentarily. Also, I would expect these two changes to solve the problem being addressed. Why is it not the case?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:963
> +            var candidateSection = this;

ditto, Section::lastSibling

[Alexander Pavlov (apavlov)]

Created attachment 106581 [details]
[PATCH] A minimally intrusive solution

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 106568 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106568&action=review
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:941
> > +            var candidateSection = this;
> 
> I'd rather introduce Section::firstSibling - it can be fetched via parent element momentarily. Also, I would expect these two changes to solve the problem being addressed. Why is it not the case?

Added first/lastSibling.

This is not the case because originally we were never able to cross the top boundary of a style with no selector (element.style). I have made a small refactoring to allow this, and dropped all my previous changes to reduce the change as much as possible.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:963
> > +            var candidateSection = this;
> 
> ditto, Section::lastSibling

Done.

[Yury Semikhatsky]

Comment on attachment 106581 [details]
[PATCH] A minimally intrusive solution

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:944
> +                if (curSection === this)

This shouldn't be needed since there is no sibling after the last section.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:950
> +        return curSection.editable ? curSection : null;

return (curSection && curSection.editable) ? curSection : null;

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:966
> +                curSection = curSection.previousSibling;

Ditto.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:970
> +        return curSection.editable ? curSection : null;

Ditto.

[Alexander Pavlov (apavlov)]

Committed r94671: <http://trac.webkit.org/changeset/94671>