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

RESOLVED FIXED in Firefox 60

Status

()

Toolkit
WebExtensions: Frontend
P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: ibrahim.alexandru, Assigned: Gijs, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla60
good-first-bug
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
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.

Updated

a year ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
(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".
Mentor: tomica
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Keywords: good-first-bug
Summary: Some bugs while creating an extension → The "editable" context isn't applied to input type="tel"
(Reporter)

Comment 2

a year ago
(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.
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

a year ago
Not sure if this should go in core:event handling (like bug 129430 comment 8) or firefox:general.  Erring into firefox:general
Component: WebExtensions: Frontend → General
Product: Toolkit → Firefox
Duplicate of this bug: 1348315

Comment 6

a year ago
The "editable" context is not applied also for "email, number, url" input fields.
Summary: The "editable" context isn't applied to input type="tel" → The "editable" context isn't applied to input type=tel, email, number, url

Comment 7

a year ago
Created attachment 8848739 [details]
Simple extension to reproduce the bug

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

Comment 8

a year ago
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?
Flags: needinfo?(tomica)
> 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
Component: General → WebExtensions: Frontend
Flags: needinfo?(tomica)
Product: Firefox → Toolkit
The WebExtension code uses the same nsContextMenu flags to determine what's editable that the Firefox frontend code does.
(Assignee)

Comment 11

a year ago
(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.
(Assignee)

Comment 12

a year ago
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.
(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...
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 14

a year ago
Created attachment 8856122 [details] [diff] [review]
bug-1345081-fix.patch
Attachment #8856122 - Flags: feedback?(tomica)
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.
Attachment #8856122 - Flags: feedback?(tomica) → feedback+

Updated

a year ago
Assignee: nobody → kernp25
Priority: -- → P5
Whiteboard: triaged
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.
Flags: needinfo?(kernp25)
Flags: needinfo?(kernp25)
This bug is open again for any contributor who wants to take it.
Assignee: kernp25 → nobody
Duplicate of this bug: 1408676

Comment 19

6 months ago
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...
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

6 months ago
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

5 months ago
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?
(Assignee)

Comment 23

5 months ago
(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

5 months ago
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

3 months ago
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?
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

3 months ago
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 hidden (mozreview-request)

Comment 30

3 months ago
mozreview-review
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.
Attachment #8942269 - Flags: review?(tomica)
Comment hidden (mozreview-request)

Comment 32

3 months ago
mozreview-review
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
Attachment #8942269 - Flags: review?(tomica) → review+

Comment 33

3 months ago
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 hidden (mozreview-request)

Comment 35

3 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5421387a997d
update use of spellchecker 'editable' flags, r=zombie
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 38

3 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c4940ee43bb
update use of spellchecker 'editable' flags, r=zombie
(Assignee)

Comment 39

3 months ago
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...
(Assignee)

Updated

3 months ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 40

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c4940ee43bb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 41

2 months ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 42

2 months ago
(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...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(tomica)
Not needed here, thanks.
Flags: needinfo?(tomica) → qe-verify-
You need to log in before you can comment on or make changes to this bug.