Closed
Bug 835808
Opened 12 years ago
Closed 11 years ago
Navigate with arrow keys in computed view
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: miker, Assigned: pbro)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Navigate with arrow keys in computed view
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•11 years ago
|
Assignee: mratcliffe → nobody
Reporter | ||
Updated•11 years ago
|
Status: ASSIGNED → NEW
Assignee | ||
Comment 1•11 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•11 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•11 years ago
|
Assignee: nobody → pbrosset
Assignee | ||
Comment 3•11 years ago
|
||
Depends on bug 914079
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 4•11 years ago
|
||
Try build at https://tbpl.mozilla.org/?tree=Try&rev=008ebbb60533
Assignee | ||
Comment 5•11 years ago
|
||
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•11 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•11 years ago
|
||
Try build is green, except for Win2012 which reports unrelated errors (it seems this platform is almost always orange).
Assignee | ||
Comment 8•11 years ago
|
||
This bug only deals with keyboard navigation. See bug 913014 for mouse interaction.
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
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•11 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)
Comment 12•11 years ago
|
||
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•11 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•11 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•11 years ago
|
||
TRY all green.
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #801562 -
Attachment is obsolete: true
Attachment #804997 -
Flags: review?(fayearthur)
Comment 17•11 years ago
|
||
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•11 years ago
|
||
Thanks for the review Heather! I'll get rid of the whitespace and attach the patch again.
Assignee | ||
Comment 19•11 years ago
|
||
Same patch as before, with trailing whitespace removed.
Attachment #804997 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 20•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•