Remove textarea binding and replace usages with html:textarea

RESOLVED FIXED in Firefox 67

Status

()

P5
normal
RESOLVED FIXED
3 months ago
17 days ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 months ago
https://searchfox.org/mozilla-central/search?q=multiline%3D%22true%22&case=true&regexp=false&path=

This should be the easier one of the textbox bindings to remove, as there are very few usages and it would be a straightforward conversion. My only worry here again is the "Edit" context menu, but that should not be a problem as only one of the 6 usages is inside a top-level window.
(Assignee)

Updated

3 months ago
Blocks: 1397874
For the context menu, I can think of three main options:
1) Enable the same context menu we use in content in the parent process
2) Use the JS Menu API like devtools does (bug 1476097)
3) Port the existing input box custom element code into a more generic thing that works for html elements

I think (1) would be the ideal option if we can make it work. This is done with nsContextMenu:
- https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/browser/base/content/tabbrowser.js#1876
- https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/browser/base/content/browser.xul#542
- https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/browser/base/content/nsContextMenu.js#84
See Also: → bug 1476097
(Assignee)

Comment 3

2 months ago
(Assignee)

Comment 4

2 months ago

Stuff that the patch still needs to do:

  • Port over the textbox.plain styling
  • Remove the resizer (resize: none) for existing usages only
  • Fix the font inheritance
  • The context menu
(Assignee)

Comment 5

2 months ago

Dão, Gijs, right now textbox[multiline] comes with a bunch of styling: https://searchfox.org/mozilla-central/search?q=.textbox-textarea&case=false&regexp=false&path=

The notable ones are resize: none; unless resizable="true" is set on the textbox, which never seems to be the case in mozilla-central; and font: inherit set in global.css. textbox[multiline].plain also gets the styling from textbox.plain which removes the background and the -moz-appearance from the textbox to make it look "plain".

I'm not sure what to do with all that styling. Would a one-to-one port of this styling to textarea in global.css be appropriate ? Should support for the "plain" class go away with the styling moving to the individual consumers ? Should textarea be styled without any class ?

What do you think ?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao+bmo)

(In reply to Tim Nguyen :ntim from comment #5)

Dão, Gijs, right now textbox[multiline] comes with a bunch of styling: https://searchfox.org/mozilla-central/search?q=.textbox-textarea&case=false&regexp=false&path=

The notable ones are resize: none; unless resizable="true" is set on the textbox, which never seems to be the case in mozilla-central;

I think we can remove support for that attribute. If we want a resizable textarea somewhere in chrome, we can opt in with CSS.

and font: inherit set in global.css.

Should probably keep this since textareas get a monospace font by default.

textbox[multiline].plain also gets the styling from textbox.plain which removes the background and the -moz-appearance from the textbox to make it look "plain".

Looks like this is only used here: https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/security/manager/pki/resources/content/certViewer.xul#113

and here: https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/toolkit/mozapps/update/content/updates.xul#145

Not sure if updates.xml is still used in Firefox. I think the cert viewer could just move to readonly text fields without the appearance dropped.

Flags: needinfo?(dao+bmo)

Comment 7

2 months ago

Dão's answered this I think.

Flags: needinfo?(gijskruitbosch+bugs)

I think we can get some solution in place for context menu. I'd say we could set up a "contextmenu" event listener on any document running in parent process that looks at e.originalTarget to decide if we should open the menu (i.e. are we an html input/textarea) and then either:

  1. Pull the context menu logic that we currently wrap up through the [context] attr on a xul element from MozInputBox out into that listener
  2. Extract the devtools menu JS module (which generates XUL menuitems behind a JS API) out into toolkit and use it. There's already a module that wraps this to generates the Edit menu, using Fluent for localization: https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-context-menu.js
Attachment #9032491 - Attachment description: Bug 1513343 - Remove textarea binding and replace usages with html:textarea. → Bug 1513343 - Remove textarea binding and replace usages with html:textarea. r=bgrins,dao
(Assignee)

Comment 11

a month ago

Hi Jeff,

I'm looking at this failure. Before I actually try and debug what's going on, is the test introduced in bug 199692 relevant to non-XUL elements ? In other words, can I remove the multiline textbox test case from that test ?

Thanks.

Flags: needinfo?(jwalden)
(Assignee)

Comment 12

a month ago

Hi Masayuki,

I'm looking at this failure. This suggests that the failure is related to the padding-left of the element, although I would like to avoid changing the padding-left of the textarea only for fixing the test.

Do you know how I can fix the failure in other ways ? (removing the textbox test, changing the expected result, ...)

Thanks.

(Assignee)

Updated

a month ago
Flags: needinfo?(masayuki)
(Assignee)

Comment 13

a month ago

Just a bit of context: this bug is changing all usages of XUL textbox[multiline="true"] to be HTML textarea.

Ah, how about to set big font-size?

Flags: needinfo?(masayuki)

...but I still don't understand why your patch caused the new failure. Doesn't it detect unexpected style change or something of <textarea>?

Or was the <textarea> an XUL's one? Even though its parent xmlns is http://www.w3.org/1999/xhtml.

(Assignee)

Comment 17

a month ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)

...but I still don't understand why your patch caused the new failure. Doesn't it detect unexpected style change or something of <textarea>?

Or was the <textarea> an XUL's one? Even though its parent xmlns is http://www.w3.org/1999/xhtml.

We're replacing the textearea here with an HTML textarea. But I guess it can just be removed since you mention HTML textareas are already tested ?

Flags: needinfo?(masayuki)

(In reply to Tim Nguyen :ntim from comment #17)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)

...but I still don't understand why your patch caused the new failure. Doesn't it detect unexpected style change or something of <textarea>?

Or was the <textarea> an XUL's one? Even though its parent xmlns is http://www.w3.org/1999/xhtml.

We're replacing the textearea here with an HTML textarea. But I guess it can just be removed since you mention HTML textareas are already tested ?

It's stored to textbox:
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/widget/tests/window_composition_text_querycontent.xul#108

And if it fails, the message has "textbox in the panel":
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/widget/tests/window_composition_text_querycontent.xul#6778

But the orange says:

widget/tests/test_composition_text_querycontent.xul | runCharAtPointTest (textarea in the document): tentative caret offset for charAtPt3 is wrong: i=1 - got 9, expected 10

It's called by here with "textarea in the document":
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/widget/tests/window_composition_text_querycontent.xul#8361
And in such case, <textarea> is tested.

Flags: needinfo?(masayuki)
(Assignee)

Comment 19

a month ago

Thanks Masayuki for your help! I think the html|textarea {font: inherit} is causing this.

As best as I recall, elementFromPoint is an API applicable to non-XUL and XUL documents both. But it's been a decade, and I am not really the right person to ask and expect a confident, correct answer. :-)

Flags: needinfo?(jwalden)
(Assignee)

Comment 21

a month ago

Enn or Boris, do you think you could answer the question from comment 11 ?

Thank you.

Flags: needinfo?(enndeakin)
Flags: needinfo?(bzbarsky)

It looks like the test is checking whether elementFromPoint returns the textbox and not the anonymous scrollbar at that point. That would still be relevant for html:textarea as well.

Flags: needinfo?(enndeakin)

Yes, agreed with comment 22.

Flags: needinfo?(bzbarsky)
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
See Also: → bug 1531119
Depends on: 1531155

Comment on attachment 9046497 [details]
Bug 1513343 - WIP Support context menus in html:textarea in the parent process

Revision D21092 was moved to bug 1531155. Setting attachment 9046497 [details] to obsolete.

Attachment #9046497 - Attachment is obsolete: true

Comment 27

17 days ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9b7d9eccb34e
Remove textarea binding and replace usages with html:textarea. r=bgrins,dao

Updated

17 days ago
Depends on: 1532595
Blocks: 1532601

Updated

17 days ago
Blocks: 1532595
No longer depends on: 1532595

That patch has a few <html:textarea readonly="true"/>, but those should be <html:textarea readonly="readonly"/>

(Assignee)

Updated

17 days ago
No longer blocks: 1532601
(Assignee)

Updated

17 days ago
Depends on: 1532632
(Assignee)

Comment 29

17 days ago

(In reply to Magnus Melin [:mkmelin] from comment #28)

That patch has a few <html:textarea readonly="true"/>, but those should be <html:textarea readonly="readonly"/>

Thanks for catching that! It seems like readonly="true" works as well, but readonly="readonly" is the standard syntax. Filed bug 1532632

Comment 30

17 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 17 days ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.