Closed Bug 1513343 Opened 3 years ago Closed 2 years ago

Remove textarea binding and replace usages with html:textarea

Categories

(Toolkit :: XUL Widgets, task, P5)

Product:
Toolkit
Component:
XUL Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1513343 - Remove textarea binding and replace usages with html:textarea. r=bgrins,dao
47 bytes, text/x-phabricator-request
Details | Review 
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.
Blocks: war-on-xbl
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: → 1476097

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

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

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)

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

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)

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.

Flags: needinfo?(masayuki)

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.

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

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)

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: → 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
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
Depends on: 1532595
Blocks: 1532601
Blocks: 1532595
No longer depends on: 1532595

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

No longer blocks: 1532601
Depends on: 1532632

(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

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Regressions: 1546367
Type: enhancement → task
Depends on: 1585884
You need to log in before you can comment on or make changes to this bug.