336799 - Remove spellchecking UI from XUL textboxes

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Adam Kowalczyk]

All chrome input fields in Firefox offer "Spell check this field" in their context menu, just like fields on websites. 

Most of the time this is useless, since the input is not suitable at all for spell checking. It looks weird for "Automatic proxy configuration URL" field. :) True, sometimes, for instance in case of search fields, it theoretically makes sense but I can't think of any case within our UI when it is actually really useful. 

Overall, I think it's unnecessary and looks strange.

Comment 1

[Brett Wilson]

Attachment: Patch

This patch never enables spellchecking in XUL single-line input boxes (even when the pref is set that causes single-lines to be checked by default). It also disables the menu for single-line input boxes so you can't turn it on.

It keeps the old behavior for multiline-controls, which is on by default, with UI to turn it off. I can only find one multi-line control in chrome, and that's in the bookmark properties dialog for descriptions.

Ideally in the future we'll have a way for XUL consumers and web pages to control spellchecking defaults so that if you wanted spellchecking in a single-line, you could turn it on or at least allow the user to turn it on (or also turn it off for multi-line). This probably won't get done for 2.0, so this patch is just trying to make the context menu not totally stupid (e.g. for the URL bar).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 222520 [details] [diff] [review]
Patch

I see no reason not to spell-check things in remote XUL, frankly.  And do we want to spell-check in chrome HTML?

That is, if we want to be checking for chrome, we should be checking for chrome (using nsContentUtils::IsChromeDoc or something), but I'm not even convinced that's true -- I can see wanting chrome that _does_ allow spellchecking, and this patch looks like it would interfere with that.

Put another way, why is the change to textbox.xml not enough for what we actually want here?

r-; I'm not going to have web access until the 29th, fwiw.

Comment 3

[Brett Wilson]

> Put another way, why is the change to textbox.xml not enough for what we
> actually want here?

The only change outside of textbox.xml was to not automatically spellcheck single-line controls ever.

Currently, if you set "layout.spellcheckDefault" = 2, you will get spellchecking on by default for both single-line and multi-line. (Normally, this pref is 1 for spellchecking multi-line only by default.)

But this also applies to XUL. This means you get wonderful things like your URL bar and the file dialog inputs spellchecked. This is wrong, and we need this change regardless of whether we *allow* spellchecking in these boxes; they should never be spellchecked by default.

In the future, we should add attributes or CSS to allow XUL consumers to change this. I doubt this will get done in time for Firefox 2, however. In the meantime, I think this patch is correct. 

Comment 4

[Simon Bünzli]

*** Bug 338995 has been marked as a duplicate of this bug. ***

Comment 5

[shutdown]

Spellchecker should be disabled in textonly textboxes.

Comment 6

[shutdown]

Oops. I meant "readonly" textboxes. For example, currently
spellchecker allows you to change the user agent string in
the firefox's about dialog. It's rather pointless.

Comment 7

[Sylvain Pasche]

I don't know if this is related, but spell checking looks like to be not working for unprivileged XUL textboxes.

I am getting "uncaught exception: Permission denied to create wrapper for object of class UnnamedClass" errors when triggering the context menu and no suggestions appear.

Should I file a separate bug ?

Comment 8

[Brett Wilson]

Yes.

Comment 9

[Sylvain Pasche]

(In reply to comment #8)
> Yes.

Opened bug 339762.

Comment 10

[Brett Wilson]

Apparently, it doesn't work for remove XUL (see bug 339762) so it should be disabled for all XUL.

Comment 11

[Martijn Wargers (dead)]

I think this could be fixed once bug 339127 gets fixed. You could then disable spellchecking on text controls by adding spellcheck="false" or something like that.
bug 339762 is more about the issue as mentioned in bug 59701.

Comment 12

[Peter Kasting]

I have a local change which fixes this

Comment 13

[Brett Wilson]

Comment on attachment 222520 [details] [diff] [review]
Patch

I should do a new patch that disables all XUL spellchecking controls.

Comment 14

[Peter Kasting]

(In reply to comment #13)
> (From update of attachment 222520 [details] [diff] [review] [edit])
> I should do a new patch that disables all XUL spellchecking controls.

Yes you should.  I don't, in fact, have a fix for this.  What I have is my patch for bug 339127, which will disable spellchecking on xul controls by default.  But it doesn't strip the UI.

I'm reassigning this to brettw.

Comment 15

[Florian Quèze [:florian]]

(In reply to comment #5 and #6)
> Spellchecker should be disabled in readonly textboxes.
> 

See bug 343687

Comment 16

[Brett Wilson]

Attachment: Patch

This patch removes the menus from XUL textboxes for controlling spellchecking. The patch for bug 339127 which should go in today changes the defaults for XUL to have no spellchecking enabled.

There are exactly two places in Firefox where spellchecking might be nice: the bookmark description that almost nobody uses, and the search box where it doesn't work. Having the menu on every element is not a good experience, and we also pay a performance price using the subscript loader to load the UI JavaScript for every textbox. If we can come up with a good experience, this patch is easy to put back.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 231114 [details] [diff] [review]
Patch

I'm fine with this change -- I'd rather remove the functionality for now, then it can be re-added later once it can be made less invasive.

Comment 18

[Brett Wilson]

Fixed on trunk. Leaving open so I remember to fix on branch.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

cc'ing shaver: This removes spellchecking UI from XUL textboxes in /content/widgets/textbox.xml for performance reasons and to get rid of menus hanging off everywhere. It's still there for text areas (off by default, as per bug 339127). I support this change, but wanted to ensure that we weren't doing something that would inappropriately limit platform/extension developers.

Comment 20

[Jed B]

Is there a reason why spell-checking doesn't work in the < editor > widget?
Any plans to add it? Or if not, why not?
Thanks.

Comment 21

[Jed B]

To answer mike, as an extension developer, it would be nice if there were easy api calls to enable/disable spell check in inputs. 
Or, if this already exists, documentation on what to call.

Comment 22

[Brian Polidoro]

Select All and the separator above it is missing from the context menu in XUL text boxes.

-        <xul:menuseparator/>
-        <xul:menuitem label="&selectAllCmd.label;" accesskey="&selectAllCmd.accesskey;" cmd="cmd_selectAll"/>
Oops!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060730 Minefield/3.0a1 ID:2006073004 [cairo]

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #22)
> Select All and the separator above it is missing from the context menu in XUL
> text boxes.
> 
> -        <xul:menuseparator/>
> -        <xul:menuitem label="&selectAllCmd.label;"
> accesskey="&selectAllCmd.accesskey;" cmd="cmd_selectAll"/>
> Oops!

Filed bug 346625 for this.

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #19)
> cc'ing shaver: This removes spellchecking UI from XUL textboxes in
> /content/widgets/textbox.xml for performance reasons and to get rid of menus
> hanging off everywhere. It's still there for text areas (off by default, as per
> bug 339127). I support this change, but wanted to ensure that we weren't doing
> something that would inappropriately limit platform/extension developers.

Quite a few developers have asked for more information on how to use the spell-checking service in their extensions, where they use at least XUL textareas pretty frequently.

Is there a reason that an explicit spellcheck="on" isn't supported for textboxes, which would select a (possibly slower) binding through the attribute selector?  Or, for that matter, turning it off via spellcheck="off" for our chrome, and getting the no-checking binding in that case.

Or the code that's just being axed out right now could check the spellcheck attribute (defaulting to "off" if not present, even) and not load the UI subscript unless it's set to "on"?  Given that it's a subscript, it could even be loaded if spellchecking is dynamically enabled, right?

(I'm curious: if we have 2 of the current textboxes and 2 of the textareas on a page, and they all have spellchecking enabled, how many copies of the subscript are we loading, compiling, and running?)

Comment 25

[Brett Wilson]

Comment on attachment 231114 [details] [diff] [review]
Patch

I need to do a new patch to avoid bug 346625

Comment 26

[Brett Wilson]

Attachment: Branch patch

This patch also fixes bug 346625 that occurred with the trunk patch above.

Comment 27

[Brett Wilson]

(In reply to comment #24)

Ideally, you would have control over spellchecking in XUL. We would really need two options: first, is spellchecking applicable (should we show the on/off UI, and what should the defaults be), and second, allow overriding of the defaults to explicitly turn it on or off.

The reason this isn't happening is that I don't have time to implement this. I'm currently stuck doing performance and crashes, and I'm not very good at XUL so it would take me a long time.

Turning it off is better than the current situation where extensions can't control it anyway, and where the user is burdened with having this extra context menu item that has absolutely no use in the base product (the only applications are the search box, where it doesn't work, and the bookmarks description that nobody uses). I can not justify doing this for something that would only be used by extensions, which like 1% of users have anyway.

I'm not sure how the subscript loader works, and how many copies we get. This is another problem with the current approach, when you right-click on a textarea it pauses (sometimes I can notice this) even on my fast computer) to lazily load the spellcheck UI subscript. I think this is unacceptable and is another reason for removing the control. For the browser content, it's associated with browser.js, so you only get one copy there.

So unless we have solutions for the performance problems and an implementation for full control over XUL, I would strongly recommend removing it. It currently hurts the product more than it helps.

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

If you're not the right person to analyze or fix the XUL-side issues, can we ask for help from someone who is?  I'm not seeking the "ideal" solution here, but I think there's a fair bit of space between the fully-controllable scenario and just ripping it out.

All I'm asking for is a way for XUL authors to make those lines run, based on them setting an attribute.  Too hard (for us, not necessarily just you) to handle dynamic attribute changes?  Then we won't support that in Fx2.  I think there's still a lot of value, and I think that because a number of people who write good extensions have asked me about it.

Can we have a textbox-with-spellcheck binding that inherits from this one, at least, so that extension authors won't end up copy-paste-and-editing their own binding?  I would really like to avoid that situation, and I don't doubt that people will do just that if we leave them no other choice.

(Not worrying about platform features because only "1%" of our users use them is a great way to keep more compelling and widely-useful extensions from appearing and raising that number; not a prophecy I'm especially interested in fulfilling.)

Comment 29

[Brett Wilson]

Sure, anybody else can help. I don't particularly care about what exactly is provided, as long it doesn't have any of the problems I outlined above.

Comment 30

[Brett Wilson]

My impression has been that if I don't do it, it won't get done, which was why I was pushing to remove this: if nobody gets around do doing something better, we don't ship in a stupid state.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231429 [details] [diff] [review]
Branch patch

a=drivers, please land this on the MOZILLA_1_8_BRANCH.

Gets us into the better state as a low-bar, and we'll file a follow up bug and see if Enn has time to help us out in time to make beta2 (see bug 346787).

Comment 32

[Brett Wilson]

Fixed on branch.