Closed Bug 835808 Opened 11 years ago Closed 11 years ago

Navigate with arrow keys in computed view

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: miker, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Navigate with arrow keys in computed view
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
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).
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: nobody → pbrosset
Depends on bug 914079
Depends on: 914079
Depends on: 913014
No longer depends on: 914079
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.
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)
Try build is green, except for Win2012 which reports unrelated errors (it seems this platform is almost always orange).
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.
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)
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!
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.
TRY all green.
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+
Thanks for the review Heather!
I'll get rid of the whitespace and attach the patch again.
Same patch as before, with trailing whitespace removed.
Attachment #804997 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/828fb3cb9b5e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: