Closed Bug 1526054 Opened 7 months ago Closed 7 months ago

Font editor prevents text from being focused in Inspector

Categories

(DevTools :: Inspector: Fonts, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox65 wontfix, firefox66 fixed, firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: dholbert, Assigned: rcaliman)

References

Details

(Keywords: regression, Whiteboard: [dt-q])

Attachments

(3 files)

STR:

  1. Visit this data URL:
data:text/html,<i>Ital</i>TryToFocusMeInInspector
  1. Open Inspector (F12) and click the "Fonts" pane on the right side.

  2. Try to focus the "TryToFocusMeInInspector" text in the left pane.

ACTUAL RESULTS:
When you attempt to focus the raw text ("TryToFocusMeInInspector"), then the <body> node ends up being focused instead. (In other words, we refuse to focus the text.)

EXPECTED RESULTS:
I should be able to focus the raw text. (Bonus points if the Fonts pane shows something useful for it. :) Presumably I can't directly restyle the text node since there's no style attribute, but the pane could at least tell me what font is being used for the text.)

Note: I get EXPECTED RESULTS (i.e. I can focus the text node) if I have a different pane focused on the right side -- something other than the Font Editor. (e.g. the Layout or Computed or Changes pane). Though, if I've got the text focused and then I switch from something else to the Font Editor pane, then the focus immediately resets to to the <body> node.

In practice, this is really annoying in scenarios where you're trying to navigate through the DOM in inspector by pressing "downarrow" over and over to skip over uninteresting nodes. If you happen to run across a node that trips this bug, then you won't ever be able to inspect past it (not by using downarrow, at least), because we skip back to the body each time you reach the text node. And it's non-obvious in practice what's causing this (the font pane being focused), so when this happens, it just feels like devtools are entirely broken.

mozregression says this started happening when bug 1485324 landed (enabling the new Font Editor) -- the old/simpler Fonts pane didn't have this problem.

Keywords: regression
Flags: needinfo?(rcaliman)
Attached file testcase 1

Here's a testcase. The raw text and the whitespace node (shown as a dot in the inspector) both trigger the issue when I try to inspect them (or just traverse past them in the inspector).

Attached video screencast of bug

(In the attached screencast, I'm trying to use my down-arrow key on my keyboard to walk across all the nodes in the inspector, but it resets me back to the <body> when I traverse across the raw text node or the whitespace "dot" node.

This only happens when the font editor panel is selected/visible.)

Whiteboard: [dt-q]

Thank you for the extensive documentation and for explaining the use case, Daniel!

It is indeed a hack to work around the constraint that font property changes currently need to be written to element inline styles. The tool operates on the selected node. Since text nodes don't support many of the Font Editor capabilities (writing to inline style, reading computed styles, etc.), we take a shortcut and select the parent HTML element node.

For the Font Editor, this solves the issue with minimal added complexity. For the use case you highlighted, navigating the Markup view by keyboard, it's clearly an issue.

The time as come to add the necessary complexity to act upon a different node than the current selection :)
I'll get to work on this.

Assignee: nobody → rcaliman
Status: NEW → ASSIGNED
Flags: needinfo?(rcaliman)
Priority: -- → P2
Summary: Font editor prevents text from being focused in Inspector (if you try, the focus jumps up to <body> instead) → Font editor prevents text from being focused in Inspector

Refactors the logic so the target node on which the Font Editor operates can point to a parent node in case of text nodes without explicitly changing the node selection.

The target node is assigned to this.node. When that is null, it means the node selection is not supported by the Font Editor. This removes the need for the isSelectedNodeValid() method.

Attachment #9047099 - Attachment description: Bug 1526054 - Set target node in Font Editor according to selection node type. r=gl → Bug 1526054 - Set target node in Font Editor according to selected node type. r=gl
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19b29b2e2f5e
Set target node in Font Editor according to selected node type. r=gl

Fixed linting issue and queued for landing again.

Flags: needinfo?(rcaliman)
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f8d66a61e23
Set target node in Font Editor according to selected node type. r=gl
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Is this worth nominating for Beta approval with Fx66 going to RC next week or can it ride the trains?

Flags: needinfo?(rcaliman)
Flags: in-testsuite+

Comment on attachment 9047099 [details]
Bug 1526054 - Set target node in Font Editor according to selected node type. r=gl

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1463084
  • User impact if declined: When the Fonts panel is open, it is impossible to traverse the Inspector's HTML markup view using a keyboard if any text nodes are rendered. When encountering a text node, the Fonts panel programmatically selects its parent element node thus preventing a user from navigating beyond it.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The code is covered by automated tests. The patch doesn't change the underlying logic in a way to impact other use cases.
  • String changes made/needed:
Flags: needinfo?(rcaliman)
Attachment #9047099 - Flags: approval-mozilla-beta?

Comment on attachment 9047099 [details]
Bug 1526054 - Set target node in Font Editor according to selected node type. r=gl

Fix for dev tools regression, has test coverage.
OK for beta 14 uplift.

Attachment #9047099 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

There is a conflict while merging devtools/client/inspector/fonts/fonts.js
Razvan , can you please take a look?

Flags: needinfo?(rcaliman)

My mistake. To avoid the merge conflict, it is necessary to first apply the patch from Bug 1523305.
I will make another beta uplift request with this correction.

Flags: needinfo?(rcaliman)

It seems I can't make another beta uplift request while the previous one is accepted.
@Liz, is is possible to change the previous one to include a dependency to the patch from Bug 1523305?

Flags: needinfo?(lhenry)

What you'd normally do is request uplift in the other bug as well. I went ahead and took care of that and approved the patch there for uplift.

Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.