1345081 - The "editable" context isn't applied to input type=tel, email, number, url

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[ibrahim.alexandru]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

I have a Chrome extension that I have ported to Firefox. So far so good, everything seems to work fine, but I did found some issues that happens on Firefox and not on Chrome and I think they cannot be fixed on my side.

1) Double click event on SELECT tags does not fire
2) Context menus created by extension, having the "contexts" attribute set to only show on "editable" fields, don't show up on inputs type=tel (probably other html5 inputs too)


Actual results:

1) tried assigning a fucntion to .ondblclick attribute, tried with jQuery "dblclick" event and does not fire no matter what. 
While trying to implement this, I also have another event "mousedown" that triggers preventDefault() on the same SELECT tag.

2) Menus created with following line: 
chrome.contextMenus.create({"contexts": ["editable"]}) don't show up on inputs with type=tel 


Expected results:

1) The event should be triggered as expected.
https://jsfiddle.net/IbrahimAbdo/syurL1hx/1/

2) Context menu should appear on all input fields, no matter what type they are.

Both issues are inexistent on Chrome.

Comment 1

[Tomislav Jovanovic :zombie]

(In reply to ibrahim.alexandru from comment #0)
> I also have another event "mousedown" that triggers preventDefault() on the same SELECT tag.

That is the reason obviously, and is how Firefox behaves in general (unrelated to extensions).  You could try filing another bug about this behavior in general, but I doubt others will agree this isn't "working as intended".

Comment 2

[ibrahim.alexandru]

(In reply to Tomislav Jovanovic :zombie from comment #1)
> (In reply to ibrahim.alexandru from comment #0)
> > I also have another event "mousedown" that triggers preventDefault() on the same SELECT tag.
> 
> That is the reason obviously, and is how Firefox behaves in general
> (unrelated to extensions).  You could try filing another bug about this
> behavior in general, but I doubt others will agree this isn't "working as
> intended".

But is there a way to have double click event on a select tag? Even if I remove the mousedown even I am not able to trigger dblclick.

Comment 3

[Tomislav Jovanovic :zombie]

Yeah, my assumption was wrong.  It looks like it only works on multiline select elements (with size>1), according to bug 129430 comment 8.

Comment 4

[:shell escalante]

Not sure if this should go in core:event handling (like bug 129430 comment 8) or firefox:general.  Erring into firefox:general

Comment 6

[jura]

The "editable" context is not applied also for "email, number, url" input fields.

Comment 7

[jura]

Attachment: Simple extension to reproduce the bug

I've added a simple extension in order to quickly reproduce the bug.

Comment 8

[:Gijs (he/him)]

Please, one issue per bug. It becomes impossible to track things otherwise. I'm assuming that the dblclick <select> thing is basically bug 129430, so let's focus on the other thing...

What's an "editable context" and why is this a Firefox bug? This is either a webextensions bug or a gecko one. :zombie, can you move this to the appropriate place?

Comment 9

[Tomislav Jovanovic :zombie]

> What's an "editable context"
It's a flag we use to let extensions add a context menu item for "editable" things on a page, so <input> fields, <textarea>s, and similar.

> and why is this a Firefox bug?
I'm not sure (I wasn't in triage), but likely because we get the flag from nsContextMenu.js[1] (or even lower, from InlineSpellChecker.jsm[2]), which look like code at the "Firefox" level.

Though it seems the spell checker isn't interested in those kinds of fields (tel, number..), so we might as well add the support for them to [1] ourselves. 

1) http://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#731
2) http://searchfox.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#442

Comment 10

[Kris Maglione [:kmag]]

The WebExtension code uses the same nsContextMenu flags to determine what's editable that the Firefox frontend code does.

Comment 11

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #10)
> The WebExtension code uses the same nsContextMenu flags to determine what's
> editable that the Firefox frontend code does.

Seems like the problem here is that nsContextMenu uses onEditableArea only for spellcheck. Basically, it seems nsContextMenu's onEditableArea really means "onSpellcheckableArea", and only gets set to true for input type=search and input type=text, that aren't readonly.

It makes sense not to display spellcheck menus on input type=tel or input type=email or input type=number or input type=url.

It doesn't make sense for webextensions to not offer 'editable' context menus for these fields.

A simple fix for this issue would be to update webextensions to check for the onTextInput flag and check the target for having readOnly set to true.

A more complicated, but perhaps/arguably more correct, fix would be to rename a bunch of these flags and have 'editable' mean something else all the way from InlineSpellChecker.jsm through the frontend code, such that the webextensions code 'magically' starts doing the right thing.

Comment 12

[:Gijs (he/him)]

It also looks like the webextensions code already works around these flags not being quite what they want, because they also check for onPassword, which I expect arguably means it does the wrong thing for readonly password fields.

Comment 13

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #11)
> Seems like the problem here is that nsContextMenu uses onEditableArea only
> for spellcheck. Basically, it seems nsContextMenu's onEditableArea really
> means "onSpellcheckableArea", and only gets set to true for input
> type=search and input type=text, that aren't readonly.
> ...
> A simple fix for this issue would be to update webextensions to check for
> the onTextInput flag and check the target for having readOnly set to true.

Ah. I guess that probably is our bug, then. Thanks. But we should probably
consider renaming some of these flags...

> A more complicated, but perhaps/arguably more correct, fix would be to
> rename a bunch of these flags and have 'editable' mean something else all
> the way from InlineSpellChecker.jsm through the frontend code, such that the
> webextensions code 'magically' starts doing the right thing.

Yeah, we should probably aim to do that after 57 when we don't have to worry
about breaking extensions. I'll file a bug.

(In reply to :Gijs from comment #12)
> It also looks like the webextensions code already works around these flags
> not being quite what they want, because they also check for onPassword,
> which I expect arguably means it does the wrong thing for readonly password
> fields.

Well, I guess it does the wrong thing for one of the two cases. I'm not sure
whether Chrome applies the editable context to readonly inputs, but people
will probably complain whichever way we decide to go...

Comment 14

[kernp25]

Attachment: bug-1345081-fix.patch

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8856122 [details] [diff] [review]
bug-1345081-fix.patch

Review of attachment 8856122 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks like a good start.  A few issues and questions:

 - Can you please switch to reviewboard, it makes it easier to mozreview[1] and keep track of changes between patches.
 - Did you run the existing tests?  Are they all passing with your changes?
 - You will need additional tests [2] to confirm this works for the input types mentioned in this bug.
 - Also, please add a test that context menus are *not* added for a readonly password field.  This should fail without your patch.


1) https://reviewboard.mozilla.org/
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#208

::: browser/base/content/nsContextMenu.js
@@ +54,5 @@
>          onImage: this.onImage,
>          onVideo: this.onVideo,
>          onAudio: this.onAudio,
>          onCanvas: this.onCanvas,
> +        onEditableArea: this.onTextInput && !this.target.readOnly,

Let's keep changes for this in web-extensions code, as other parts of Firefox, or other non-webextension addons might depend on existing behavior.

Comment 16

[Tomislav Jovanovic :zombie]

Hi @kernp25, are you still working on this?  Did you maybe get stuck on something?

Please feel free to ask for any help if needed.

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

This bug is open again for any contributor who wants to take it.

Comment 19

[u593386]

This breaks the extension I've been contracted to work on, as the menu item is used to paste an email address into the field, and the one place it is not appearing is the email fields...

Comment 20

[Tomislav Jovanovic :zombie]

If you are familiar with developing firefox extensions, this might be a fairly easy bug to try and contribute, if it's important to you, and I'd be happy to mentor.  

If you have any questions, feel free to ask.

Comment 21

[u593386]

I'm only familiar with web extensions, as the extension I've been working on already had a Chrome version, so it's been easier to work from that than from the FF version. If web extension knowledge is the only requirement, I might be able to have a quick look, otherwise at the moment, the best solution for us would probably be to change to the 'all' context until this is fixed, though that obviously damages the UX a little.

Comment 22

[Sam Bull]

So, we're receiving bug reports from users less than a week after releasing the new web extension, thanks to this bug. Where is the code that handles this?

Comment 23

[:Gijs (he/him)]

(In reply to Sam Bull from comment #22)
> So, we're receiving bug reports from users less than a week after releasing
> the new web extension, thanks to this bug. Where is the code that handles
> this?

See the attached patch. The general context menu handling code is in browser/base/content/nsContextMenu.js, the webextension stuff is in browser/components/extensions/ext-contextMenus.js . All of the code lives on https://hg.mozilla.org/mozilla-central/ , and the top section in https://developer.mozilla.org/en-US/docs/Simple_Firefox_build should help you get set up. #introduction on our IRC server (irc.mozilla.org) will have people who can help you out if you get stuck.

Comment 24

[Sam Bull]

I'm not really sure exactly how much the SpellChecker stuff works, but at a first glance it looks like you could just add a flag here: https://hg.mozilla.org/mozilla-central/file/tip/toolkit/modules/InlineSpellChecker.jsm#l445

    flags |= this.INPUT;
    if (!element.readOnly)
      flags |= this.EDITABLECONTEXT

Then change the check at: https://hg.mozilla.org/mozilla-central/file/tip/browser/modules/ContextMenu.jsm#l877

    context.onEditableArea = (editFlags & SpellCheckHelper.EDITABLECONTEXT) !== 0;


I'm assuming that would be enough to avoid changing the spell checker behaviour.

Comment 25

[sami.vanttinen]

I also ran to this bug today. Is the fix coming along or is the only viable solution for this to change contextType to 'all' until this is resolved?

Comment 26

[Tomislav Jovanovic :zombie]

This bug is P5, meaning no one currently plans to work on it, but we will accept code contributions from the community.

(In reply to Sam Bull from comment #24)
> I'm not really sure exactly how much the SpellChecker stuff works, but at a
> first glance it looks like you could just add a flag here:
> https://hg.mozilla.org/mozilla-central/file/tip/toolkit/modules/
> InlineSpellChecker.jsm#l445
> 
>     flags |= this.INPUT;
>     if (!element.readOnly)
>       flags |= this.EDITABLECONTEXT

That wouldn't exactly work, as those flags are used by other parts of Firefox (at least the spell checker), and a change like that would break them.  Some suggested solutions are described in comment 11.

Comment 27

[sami.vanttinen]

For a quick solution the following should also work:

Enable editable for context menus with any input field detected (btw, WebKit based browsers do this almost identically):
https://hg.mozilla.org/mozilla-central/file/tip/browser/modules/ContextMenu.jsm#l877

      context.onEditableArea = (editFlags & SpellCheckHelper.INPUT) !== 0;

And only enable spell checking for clearly EDITABLE types;
https://hg.mozilla.org/mozilla-central/file/tip/browser/modules/ContextMenu.jsm#l880

      if (editFlags & SpellCheckHelper.EDITABLE) {

Could this still break something? In my opinion doing this properly would mean not to have context menus rely on spell checking.

Comment 28

[:Gijs (he/him)]

So, there's not that many uses of `EDITABLE`. Let's just make it mean what it says it means, and update the other cases:

https://dxr.mozilla.org/mozilla-central/search?q=EDITABLE+-path%3Aaccessible+-path%3Aeditor%2F+-NODE_IS_EDITABLE+-path%3Amodules%2Ffreetype+-path%3Aobj-x86+-path%3Aother-licenses+-path%3Aparser%2Fhtml+-path%3Aservo%2Fports%2F+-path%3Atesting%2Fweb-platform+-path%3Adevtools+-path%3Athird_party%2Frust+path%3Awidget%2Fgtk&redirect=false

Comment 29

[:Gijs (he/him)]

Attachment: Bug 1345081 - update use of spellchecker 'editable' flags,

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

Comment 30

[Tomislav Jovanovic :zombie]

Comment on attachment 8942269 [details]
Bug 1345081 - update use of spellchecker 'editable' flags,

https://reviewboard.mozilla.org/r/212556/#review218428

Thanks for taking this, mostly looks good.

I'm not that familiar with the non-webextension parts, but it seems fine to me, except for the new EDITABLE logic.  And if I'm right, and all our tests pass with this code, please add one that doesn't.  ;)

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:242
(Diff revision 1)
> +
> +  // Check some menu items.
> +  items = extensionMenuRoot.getElementsByAttribute("label", "editable");
> +  is(items.length, 0, "contextMenu item for text input element was not found (context=editable fails for readonly items)");
> +
> +  // Hide the popup "manually" because there's nothing to click.

I think `closeContextMenu()` from `head.js` should work here.

::: browser/modules/ContextMenu.jsm:881
(Diff revision 1)
>        context.onNumeric = (editFlags & SpellCheckHelper.NUMERIC) !== 0;
> -      context.onEditableArea = (editFlags & SpellCheckHelper.EDITABLE) !== 0;
> +      context.onEditable = (editFlags & SpellCheckHelper.EDITABLE) !== 0;
>        context.onPassword = (editFlags & SpellCheckHelper.PASSWORD) !== 0;
> +      context.onSpellcheckable = (editFlags & SpellCheckHelper.SPELLCHECKABLE) !== 0;
>  
> -      if (context.onEditableArea) {
> +      if (context.onEditable) {

Shouldn't this be `onSpellcheckable`?

::: browser/modules/ContextMenu.jsm:1015
(Diff revision 1)
>      }
>  
>      // if the document is editable, show context menu like in text inputs
> -    if (!context.onEditableArea) {
> +    if (!context.onEditable) {
>        if (editFlags & SpellCheckHelper.CONTENTEDITABLE) {
>          // If this._onEditableArea is false but editFlags is CONTENTEDITABLE, then

Please update this comment as well.

::: toolkit/modules/InlineSpellChecker.jsm:387
(Diff revision 1)
>        this.mInlineSpellChecker.ignoreWord(this.mMisspelling);
>    }
>  };
>  
>  var SpellCheckHelper = {
>    // Set when over a non-read-only <textarea> or editable <input>.

Is this comment still correct?

::: toolkit/modules/InlineSpellChecker.jsm:450
(Diff revision 1)
>  
>    isEditable(element, window) {
>      var flags = 0;
>      if (element instanceof window.HTMLInputElement) {
>        flags |= this.INPUT;
> +      if (!element.readOnly) {

I don't think this is correct.  What about checkboxes, radios, buttons and possibly other input types?

::: toolkit/modules/InlineSpellChecker.jsm:479
(Diff revision 1)
>        if (!element.readOnly) {
> -        flags |= this.EDITABLE;
> +        flags |= this.SPELLCHECKABLE | this.EDITABLE;
>        }
>      }
>  
> -    if (!(flags & this.EDITABLE)) {
> +    if (!(flags & this.SPELLCHECKABLE)) {

This is probably ok, but I'm a bit confused.  A reminder for me to go over this (and the corresponding code in ContextMenu.jsm) again after the final EDITABLE logic is in place.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8942269 [details]
Bug 1345081 - update use of spellchecker 'editable' flags,

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

Comment 32

[Tomislav Jovanovic :zombie]

Comment on attachment 8942269 [details]
Bug 1345081 - update use of spellchecker 'editable' flags,

https://reviewboard.mozilla.org/r/212556/#review220874

::: toolkit/modules/tests/browser/browser_InlineSpellChecker.js:141
(Diff revision 2)
> +      "tel": INPUT | EDITABLE | TEXTINPUT,
> +      "email": INPUT | EDITABLE | TEXTINPUT,
> +      "number": INPUT | EDITABLE | TEXTINPUT | NUMERIC,
> +      "checkbox": INPUT,
> +      "radio": INPUT,
> +    };

nice

Comment 33

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 44b6e3fb98aff3e88394d8d9d879b99dcf8be272 -d 21d199596afd: rebasing 444066:44b6e3fb98af "Bug 1345081 - update use of spellchecker 'editable' flags, r=zombie" (tip)
merging browser/components/extensions/ext-menus.js
warning: conflicts while merging browser/components/extensions/ext-menus.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 34

[:Gijs (he/him)]

Comment on attachment 8942269 [details]
Bug 1345081 - update use of spellchecker 'editable' flags,

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5421387a997d
update use of spellchecker 'editable' flags, r=zombie

Comment 36

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome browser/components/extensions/test/browser/browser_ext_menus_events.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5421387a997d4ed4dfb3238aa59f34dd371dd305

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158687961&repo=autoland&lineNumber=20778


Backout: https://hg.mozilla.org/integration/autoland/rev/831d2c0e7bf95bba7f8a2ae4c3a85cf05e565e5b

Comment 37

[:Gijs (he/him)]

Comment on attachment 8942269 [details]
Bug 1345081 - update use of spellchecker 'editable' flags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212556/diff/3-4/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c4940ee43bb
update use of spellchecker 'editable' flags, r=zombie

Comment 39

[:Gijs (he/him)]

Relanded with updated expectations for that test, which fixes the failures for me locally.

Still kinda confused because I ran the patch through try and the failures that happened when pushing to autoland don't show up there...

Comment 40

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2c4940ee43bb

Comment 41

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 42

[:Gijs (he/him)]

(In reply to marius.santa from comment #41)
> Is manual testing required on this bug? If Yes, please provide some STR and
> the proper webextension(if required), if No set the “qe-verify-“ flag.

:zombie knows better than me whether it's useful to have this...

Comment 43

[Tomislav Jovanovic :zombie]

Not needed here, thanks.