1155465 - Odd navigation problems editing a multiline text node with numbers at the beginning of the line

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

STR:

1. go to this bug, open the inspector
2. select #comment_text_0, expand to see the text node, select the text and hit enter to edit
3. in text edit mode, hit arrow down to de-select al the text, then CMD+ left arrow to send the cursor to column 0, the curso should now be at this line:

"2. Press Enter              Nothing happens"

4. arrow up on the keyboard

Expected: cursor goes up
Actual: the number is incremented

Comment 1

[Patrick Brosset <:pbro>]

Whoa, that's really weird, and very frustrating, you can't navigate the text anymore with the cursor, everytime you're close to a number, the up/down arrows are used to increment/decrement that number, and you can't use them anymore to navigate.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Adding devedition-40 flag and setting p2. Now that we know about this I'd hate for someone to run into it in the wild, but it's not the most impactful thing in the list.

Comment 3

[Patrick Brosset <:pbro>]

Flagging this as easy because I think we should fix this simply by not doing any numbers incrementing/decrementing when the inplace-editor instance is multiline. I just don't see a good way to navigate multiline fields if the up and down arrows are hijacked to do something else when around a number.

Also, it seems like only the TextEditor class (in markup-view.js) uses the multiline:true config param, to edit TextNodes content. I don't think we'd miss having this feature there. Although it makes a lot of sense when editing attributes or css rules, it's not as useful when editing textcontent.

Comment 4

[Julian Descottes [:jdescottes]]

Linked to Bug 1143742. 

We will now use a multiline editor for the rule-view property values in order to support editing "long properties". 

Therefore, we can't completely disable incrementing/decrementing/autocompletion based on the fact that the editor is multiline.

We can either:
- make this configurable 
- or add some logic to detect on which line the user is. If pressing UP/DOWN can move the caret to another line, then do not increment/decrement/autocomplete

Comment 5

[Julian Descottes [:jdescottes]]

Alternatively, I think we should use the "content type" of the inplace editor in order to enable/disable the feature. There are four content types as of now:
- PLAIN_TEXT
- CSS_VALUE
- CSS_MIXED
- CSS_PROPERTY

The content type PLAIN_TEXT is used only for editing:
- selectors (rule-view)
- text nodes (markup)
- comment nodes (markup)

I think disabling the feature for all 3 is acceptable. Patch coming up.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1155465 - part2: inplace-editor: disable increment on up/down for PLAIN_TEXT;r=miker

This patch forces the increment step to 0 when the type of the inplace-editor is
PLAIN_TEXT.

This type is used by the markup view for editing text and comment nodes, as well
as by the rule-view to edit selectors.

All other inplace editors will still benefit from the increment/decrement feature.

Review commit: https://reviewboard.mozilla.org/r/38991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38991/

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1155465 - part1: layout-view: use content-type CSS_VALUE for editors;r=miker

Inplace editors for the layout view should use the CSS_VALUE content-type since they
allow to edit a CSS value, and not just plain text.

Review commit: https://reviewboard.mozilla.org/r/39161/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39161/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8728609 [details]
MozReview Request: Bug 1155465 - part2: inplace-editor: disable increment on up/down for PLAIN_TEXT;r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38991/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

As seen on the try test, there were failures in browser_layout_editablemodel.js . The layout view was also using plain text editors. Since they are editing css properties, I added a first changeset to change the content type to CSS_VALUE, with the appropriate property name.

The inplace editor is also used in the projecteditor, but only for creating and editing file names. Having no increment here should not be an issue.

Restarted a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3900924a4953

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8728833 [details]
MozReview Request: Bug 1155465 - part1: layout-view: use content-type CSS_VALUE for editors;r=miker

https://reviewboard.mozilla.org/r/39161/#review35811

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8728609 [details]
MozReview Request: Bug 1155465 - part2: inplace-editor: disable increment on up/down for PLAIN_TEXT;r=miker

https://reviewboard.mozilla.org/r/38991/#review35813

r+ assuming green try.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/971c1817df4c
https://hg.mozilla.org/integration/fx-team/rev/7feee955891b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/971c1817df4c
https://hg.mozilla.org/mozilla-central/rev/7feee955891b