Navigate with arrow keys in computed view

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: miker, Assigned: pbro)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Navigate with arrow keys in computed view
Assignee: nobody → mratcliffe
Blocks: 831711
Status: NEW → ASSIGNED
Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 1

4 years ago
Today, keyboard navigation in the computed view works with TAB/Shift-TAB to move down/up, and ENTER to toggle matched selectors. However, there is absolutely no visual feedback on this, so it's impossible to tell which line is currently selected.

If we wanted to make it consistent with the markup view, arrow UP/DOWN should be used instead. But in that case, the rules view should also work that way (it works with TAB/Shift-TAB now).
(Assignee)

Comment 2

4 years ago
For information, both Firebug and Chrome have a TAB-based navigation in the rules view, which makes sense knowing that UP/DOWN arrows are used to increment/decrement CSS values. And both of them have no keyboard navigation in the computed view at all.

So, as a result, what I would propose is to keep the navigation handling we currently have but add a visual representation of focus in the computed view.
(Assignee)

Updated

4 years ago
Assignee: nobody → pbrosset
(Assignee)

Comment 3

4 years ago
Depends on bug 914079
(Assignee)

Updated

4 years ago
Depends on: 914079
(Assignee)

Updated

4 years ago
Depends on: 913014
No longer depends on: 914079
(Assignee)

Comment 5

4 years ago
Created attachment 801562 [details] [diff] [review]
835808-show-focused-computed-view.patch

This patches basically moves the tabindex and keydown handler on the element rather than just on the twisty.

I also took the opportunity to re-organize and comment a bit more the buildMain function.

Finally, I saw that 90% of the CSS code was duplicated in the osx, linux and windows themes, so I moved the common code to 1 single CSS file.
(Assignee)

Comment 6

4 years ago
Heather, could you please advise if the move to the common CSS is correct, or if the code should go into the shared theme CSS files ?
Flags: needinfo?(fayearthur)
(Assignee)

Comment 7

4 years ago
Try build is green, except for Win2012 which reports unrelated errors (it seems this platform is almost always orange).
(Assignee)

Comment 8

4 years ago
This bug only deals with keyboard navigation. See bug 913014 for mouse interaction.
Right no(In reply to Patrick Brosset from comment #6)
> Heather, could you please advise if the move to the common CSS is correct,
> or if the code should go into the shared theme CSS files ?

Unfortunately, even if there's duplicate CSS, we seem to leave it in the OS files. The common central file is for CSS that wouldn't ever need to be tweaked for a platform, like "visibility:hidden". Anything to do with spacing, dimensions, colors, etc. goes in the OS files.
Flags: needinfo?(fayearthur)
Additionally, I think CSS in non-theme/<os> CSS can't be overriden by themes, which is one reason why we have everything in there.
(Assignee)

Comment 11

4 years ago
Thanks for the feedback Heather.
I understand the distinction now. However, I'm a bit puzzled about some of the properties then. Here are a few example:

> body {
>   margin: 0;
>   display : flex;
>   flex-direction: column;
>   height: 100%;
> }

and

> .property-name {
>   width: 50%;
>   overflow-x: hidden;
>   text-overflow: ellipsis;
>   white-space: nowrap;
>   outline: 0;
> }

And more. 
For me, these look like "structural" CSS code that doesn't need to be part of the themes. I don't see these rules ever needing to be tweaked for a particular platform neither needing to be customized by a theme.

Please let me know if my understanding is correct in which case I can split the CSS, putting in the theme/os files everything that has to do with colors, images, spacing, ... and putting in the new common file everything that deals with positioning and layout (structure).

Thanks
Flags: needinfo?(fayearthur)
Most of those, like margin, width, and overflow might need to be changed for a platform or theme, even text-overflow:ellipsis. Sorry, I wasn't comprehensive enough in my list )= This is mainly for congruency with the rest of our code. We discussed centralizing, but we decided against it for now. Sorry.
Flags: needinfo?(fayearthur)
(Assignee)

Comment 13

4 years ago
Thanks Heather. If it's been discussed already, then I'm completely fine with the decision.
I'll do the changes, launch a build again and ask for a final review then.
Thanks!
(Assignee)

Comment 14

4 years ago
Changes done. The CSS code is back in the platform theme files.
TRY build ongoing at https://tbpl.mozilla.org/?tree=Try&rev=05bbf1607501
Will upload a patch for review as soon as build is done/green.
(Assignee)

Comment 15

4 years ago
TRY all green.
(Assignee)

Comment 16

4 years ago
Created attachment 804997 [details] [diff] [review]
835808-show-focused-computed-view.patch
Attachment #801562 - Attachment is obsolete: true
Attachment #804997 - Flags: review?(fayearthur)
Comment on attachment 804997 [details] [diff] [review]
835808-show-focused-computed-view.patch

Review of attachment 804997 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Patrick.

::: browser/devtools/styleinspector/test/browser_computedview_bug835808_keyboard_nav.js
@@ +59,5 @@
> +
> +function testTabThrougStyles()
> +{
> +  let span = doc.querySelector("span");
> +  

super nit: trailing whitespace. Can fix before check in.
Attachment #804997 - Flags: review?(fayearthur) → review+
(Assignee)

Comment 18

4 years ago
Thanks for the review Heather!
I'll get rid of the whitespace and attach the patch again.
(Assignee)

Comment 19

4 years ago
Created attachment 806447 [details] [diff] [review]
835808-show-focused-computed-view.patch

Same patch as before, with trailing whitespace removed.
Attachment #804997 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/828fb3cb9b5e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/828fb3cb9b5e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.