Closed Bug 1120406 Opened 9 years ago Closed 9 years ago

Visualize that an element has locked pseudo-classes within markup view

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox43 fixed, relnote-firefox 43+)

Status:
VERIFIED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- fixed
relnote-firefox --- 43+

People

(Reporter: sebo, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(2 files, 2 obsolete files)

MozReview Request: Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl
40 bytes, text/x-review-board-request
gl
: review+
gl
: feedback+
Details
pseudoclass-lock-dots.png
146.31 KB, image/png
Details 
The markup view should indicate when an element has a pseudo-class locked.

Note that the Chrome devtools have a nice solution for that by displaying an orange circle at the left border of the markup view besides the element. In case the parent of the element is collapsed the circle is displayed a bit more pale.
Hovering the circle shows a tooltip indicating the locked states or on parents the number of descendants with locked state.

Sebastian
This is WIP that sets the background color of the locked pseudo class nodes in the markup view.  The UI should change to a little dot or something
See Also: → 987365
Blocks: 985517
See Also: 985517 →
Whiteboard: [devedition-40][difficulty=easy]
Priority: -- → P2
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Attached patch pseudo-class-lock-UI.patch (obsolete) — — Splinter Review 
rebased and using a circle to the left of the container instead of a background color
Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl
Attachment #8547646 - Attachment is obsolete: true
Attachment #8653013 - Attachment is obsolete: true
Comment on attachment 8653072 [details]
MozReview Request: Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl

Gabriel, can you have a look at this?  Interested in your feedback
Attachment #8653072 - Flags: feedback?(gabriel.luong)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8653072 - Flags: feedback?(gabriel.luong) → feedback+
Comment on attachment 8653072 [details]
MozReview Request: Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl

https://reviewboard.mozilla.org/r/17353/#review15893
Attachment #8653072 - Flags: review+
Comment on attachment 8653072 [details]
MozReview Request: Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl

Patrick, gl has reviewed this but just want to make sure you've seen this and have a chance to give any feedback
Attachment #8653072 - Flags: review?(pbrosset)
Screenshot with patch applied
Comment on attachment 8653072 [details]
MozReview Request: Bug 1120406 - Indicate which nodes have a pseudo-class lock applied in the markup view;r=gl

https://reviewboard.mozilla.org/r/17353/#review15961

::: browser/devtools/markupview/markup-view.js:1826
(Diff revision 1)
> +    return this.node._parent === this.markup.walker.rootNode;

Why should the <html> node not be expandable? It currently is. 
Is it because we need some space to display the pseudo-class icon?
It feels odd to remove this feature (even if probably not used often) for this. I understand though that indenting the whole tree in order to fit the icon would look bad.
Did you consider other solutions?
Attachment #8653072 - Flags: review?(pbrosset)
I just checked on Chrome (release): the html node can be expanded and the lock icon is displayed on top of the expand icon, which is weird.
On Chrome (canary): the html node can't be expanded anymore, so they've made the same change you propose.
Chrome Canary works different. The root node cannot be collapsed anymore. I.e. the expand icon is gone giving more space for the lock icon (and the break on modification icon).

From comment #8 I guess that's the same what's in the patch here. And I believe that's fine. There is probably no big use case to be able to collapse the <html> node.

Sebastian
Comment on attachment 8655123 [details]
pseudoclass-lock-dots.png

I think we should unify the color code and use the same color in the rules view for highlighting pseudo classes in selectors and the markup view.
(In reply to Tim Nguyen [:ntim] from comment #11)
> Comment on attachment 8655123 [details]
> pseudoclass-lock-dots.png
> 
> I think we should unify the color code and use the same color in the rules
> view for highlighting pseudo classes in selectors and the markup view.

I think this is a great idea. I am also wondering about the highlight colour we use in the rule view to highlight the pseudo classes in the selectors.
(In reply to Gabriel Luong [:gl] from comment #12)
> (In reply to Tim Nguyen [:ntim] from comment #11)
> > Comment on attachment 8655123 [details]
> > pseudoclass-lock-dots.png
> > 
> > I think we should unify the color code and use the same color in the rules
> > view for highlighting pseudo classes in selectors and the markup view.
> 
> I think this is a great idea. I am also wondering about the highlight colour
> we use in the rule view to highlight the pseudo classes in the selectors.

Yeah I tried switching to theme-highlight-orange in the markup view for the dots and breadcrumbs, and it doesn't work well.  I think we may want to switch the rule view color instead, but let's file a follow up bug to review the UI there.
Blocks: 1200686
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Yeah I tried switching to theme-highlight-orange in the markup view for the
> dots and breadcrumbs, and it doesn't work well.  I think we may want to
> switch the rule view color instead, but let's file a follow up bug to review
> the UI there.

Filed Bug 1200686
https://hg.mozilla.org/mozilla-central/rev/83844eb33969
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Keywords: dev-doc-needed
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Add more visual indicators to node information in the markup view
[Links (documentation, blog post, etc)]:

Is that too vague? Brian, what wording would you prefer? We can also just use the bug summary if that's better.
relnote-firefox: --- → 43+
Flags: needinfo?(bgrinstead)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Release Note Request (optional, but appreciated)
> [Why is this notable]:
> [Suggested wording]: Add more visual indicators to node information in the
> markup view
> [Links (documentation, blog post, etc)]:
> 
> Is that too vague? Brian, what wording would you prefer? We can also just
> use the bug summary if that's better.

How about "Show indicators in the markup view next to nodes that have pseudo-class locks set"
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> > Release Note Request (optional, but appreciated)
> > [Why is this notable]:
> > [Suggested wording]: Add more visual indicators to node information in the
> > markup view
> > [Links (documentation, blog post, etc)]:
> > 
> > Is that too vague? Brian, what wording would you prefer? We can also just
> > use the bug summary if that's better.
> 
> How about "Show indicators in the markup view next to nodes that have
> pseudo-class locks set"

I believe the text should not be in imperative. So I'd say "Markup view shows indicators for pseudo-classes locked for elements".

Sebastian
Status: RESOLVED → VERIFIED
I've added a note on this:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Setting_hover_active_focus

Let me know if it works for you.
Flags: needinfo?(bgrinstead)
Needinfo'ing Liz for comment 20.

(In reply to Will Bamberg [:wbamberg] from comment #21)
> I've added a note on this:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Setting_hover_active_focus
> 
> Let me know if it works for you.

Nit from my side: I wouldn't be that specific that the dot is orange to avoid having to change the documentation in case the color gets changed in the future.

Also listed this feature at https://developer.mozilla.org/en-US/Firefox/Releases/43.

Sebastian
Flags: needinfo?(lhenry)
(In reply to Sebastian Zartner [:sebo] from comment #22)
> Needinfo'ing Liz for comment 20.
> 
> (In reply to Will Bamberg [:wbamberg] from comment #21)
> > I've added a note on this:
> > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> > Examine_and_edit_CSS#Setting_hover_active_focus
> > 
> > Let me know if it works for you.
> 
> Nit from my side: I wouldn't be that specific that the dot is orange to
> avoid having to change the documentation in case the color gets changed in
> the future.
> 
> Also listed this feature at
> https://developer.mozilla.org/en-US/Firefox/Releases/43.
> 
> Sebastian

There's a screenshot, so I'm going to have to anyway :/
The note looks good, thanks Will.  I agree that the 'orange' part might change - in fact we might do that in Bug 1205371.  But doesn't seem there is a way around it for now and I'll make sure we mark dev-doc-needed on that once we make any changes.
Blocks: 1205371
Flags: needinfo?(bgrinstead)
OK, the note now reads "	Markup view shows indicators for pseudo-classes locked for elements" and links to https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Setting_hover_active_focus
Flags: needinfo?(lhenry)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: