1513343 - Remove textarea binding and replace usages with html:textarea

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Tim Nguyen :ntim]

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.

Comment 1

[Brian Grinstead [:bgrins]]

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

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1513343 - Remove textarea binding and replace usages with html:textarea. r=bgrins,dao

Comment 3

[Tim Nguyen :ntim]

This needs to be undone: https://phabricator.services.mozilla.com/D16334

Comment 4

[Tim Nguyen :ntim]

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

Comment 5

[Tim Nguyen :ntim]

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 ?

Comment 6

[Dão Gottwald [:dao]]

(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.

Comment 7

[:Gijs (he/him)]

Dão's answered this I think.

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c7283ab0fa8e8a3e5034b9f24e25ee13b54e103

Comment 10

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb1ab97efeb5056bafe2d0d073ad18e12ebb2e0c

Comment 11

[Tim Nguyen :ntim]

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.

Comment 12

[Tim Nguyen :ntim]

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.

Comment 13

[Tim Nguyen :ntim]

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

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

That's odd. According to the log, it tests the behavior in <textarea>:
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/widget/tests/window_composition_text_querycontent.xul#28,106,8361
I don't understand what happened...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, how about to set big font-size?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

...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.

Comment 17

[Tim Nguyen :ntim]

(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 ?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 19

[Tim Nguyen :ntim]

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

Comment 20

[Jeff Walden [:Waldo]]

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. :-)

Comment 21

[Tim Nguyen :ntim]

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

Thank you.

Comment 22

[Neil Deakin]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yes, agreed with comment 22.

Comment 24

[Tim Nguyen :ntim]

All try failures should be fixed now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01cae0ecc7f346aab690e9fa35581116a5e62983

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: Bug 1513343 - WIP Support context menus in html:textarea in the parent process

Comment 26

[Phabricator Automation]

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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b7d9eccb34e
Remove textarea binding and replace usages with html:textarea. r=bgrins,dao

Comment 28

[Magnus Melin [:mkmelin]]

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

Comment 29

[Tim Nguyen :ntim]

(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

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9b7d9eccb34e