1336320 - Make increaseFontSize / decreaseFontSize noscriptable

Status: RESOLVED DUPLICATE of bug 1611751

(Core :: DOM: Editor, defect, P3)

Description

[Makoto Kato [:m_kato]]

All addons doesn't use increaseFontSize and decreaseFontSize of nsIHTMLEditor.  So we should mark as noscript.

Also, we already has cmd_increaseFont and cmd_decreaseFont by execCommands as same method.  So if someone wants to use these methods, they can use exeCommands instead.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1336320 - Mark increaseFontSize / decreaseFontSize as noscript.

Both methods are called from execCommands only, Although I check all existed addons, they uses execCommands instead of nsIHTMLEditor methods directly.  We already provide same functions by execCommands, so we should mark as noscript.

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

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Please don't do that. BlueGriffon uses all the styling methods on nsIHTMLEditor including these ones.

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Daniel Glazman (:glazou) from comment #2)
> Please don't do that. BlueGriffon uses all the styling methods on
> nsIHTMLEditor including these ones.

You can use cmd_increaseFont and cmd_decreaseFont instead of this method.  Why can not BlueGriffon change to execCommands("cmd_increaseFont") instead of increaseFontSize and decreaseFontSize ?

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Makoto Kato [:m_kato] from comment #3)
> (In reply to Daniel Glazman (:glazou) from comment #2)
> > Please don't do that. BlueGriffon uses all the styling methods on
> > nsIHTMLEditor including these ones.
> 
> You can use cmd_increaseFont and cmd_decreaseFont instead of this method. 
> Why can not BlueGriffon change to execCommands("cmd_increaseFont") instead
> of increaseFontSize and decreaseFontSize ?

First because this is not at all how BlueGriffon works. It's not a webapp,
it's a standalone application embedding Gecko. BlueGriffon deeply relies on
nsIHTMLEditor and even extends it. Calling execCommand() can return w/o action if
the corresponding command is currently disabled. An embedding client may need
to call a scriptable method from nsIHTMLEditor *even if* the corresponding
command - that is more related to User interaction than embedding - is marked
as currently disabled.

If these two nsIHTMLEditor methods become non-scriptable, then almost nothing in that
interface deserves to remain scriptable.

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

sorry, hit "Save Changes" too early.

and in that case, the few remaining applications that embed Gecko will
not be able to do it any more.

Comment 6

[Makoto Kato [:m_kato]]

If embedded gecko and you want to check error, I think you can call nsIControllerCommandTable.DoCommand("cmd_increaseFont") instead.  Now, since XPCOM interfaces are non-frozen,  XPCOM interface sometimes is changed.  If many addons use it, it isn't changed.

Comment 7

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

Using command controller makes sense to me. Although, even if there is no available editor, it could return NS_OK:
https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/editor/composer/nsComposerCommands.cpp#1206,1212-1213
Isn't it in the command table when there is no HTML editor? Or should we fix it here too?

Comment 8

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

According to checking the docshell roughly, HTMLEditor is created at first time becoming necessary. However, it's not detached from the docshell until the page is hidden.

Comment 9

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

Oops, so, I meant that, the caller anyway to check if it's editable with isCommandEnabled before calling doCommand if the caller needs to check the command will do something exactly.

Comment 10

[Makoto Kato [:m_kato]]

Ah, we can simply access controller like document.commandDispatcher.getControllerForCommand("cmd_increaseFont").doCommand("cmd_increaseFont")

Comment 11

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

Comment on attachment 8846493 [details]
Bug 1336320 - Mark increaseFontSize / decreaseFontSize as noscript.

https://reviewboard.mozilla.org/r/119536/#review121830

So, same function as these methods is available with using nsIControllerCommand.doCommand and nsIControllerCommand.isCommandEnabled. So, doesn't make sense to keep them scriptable. They should be removed after making nsIEditor builtinclass.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

You're *seriously* proposing to replace |editor.increaseFontSize()| by |editor.document.commandDispatcher.getControllerForCommand("cmd_increaseFont").doCommand("cmd_increaseFont")| ?

I still STRONGLY disagree with this bug for the reasons outlined above. A command
is different from an API. We need the API even if the command is disabled.

Comment 13

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

(In reply to Daniel Glazman (:glazou) from comment #12)
> You're *seriously* proposing to replace |editor.increaseFontSize()| by
> |editor.document.commandDispatcher.
> getControllerForCommand("cmd_increaseFont").doCommand("cmd_increaseFont")| ?
> 
> I still STRONGLY disagree with this bug for the reasons outlined above. A
> command
> is different from an API. We need the API even if the command is disabled.

It's not clear to me. |nsIncreaseFontSizeCommand::DoCommand()| just calls |nsIHTMLEditor::IncreaseFontSize()| without checking if it's enabled. What's the point you don't think they are same?

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Masayuki Nakano [:masayuki] from comment #13)

> It's not clear to me. |nsIncreaseFontSizeCommand::DoCommand()| just calls
> |nsIHTMLEditor::IncreaseFontSize()| without checking if it's enabled. What's
> the point you don't think they are same?

You're correct, |nsIncreaseFontSizeCommand::DoCommand| doesn't check the enabled/
disabled state of the command, my bad. But you originally wanted to replace this
API by an |execCommand()| call and execCommand *DOES* verify if the command is
enabled or disabled in

https://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#3231

Furthermore, I am still saying switching from a direct, simple, easy to read,
easy to maintain API on the editor to an awful, long, complex, error-prone
call through the commandDispatcher, the controller for a command, the command itself
is overkill. Such a call is basically unworkable so all users and embedders will
recreate a simpler API. In other terms, the proposed change is of little value.

Comment 15

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

(In reply to Daniel Glazman (:glazou) from comment #14)
> (In reply to Masayuki Nakano [:masayuki] from comment #13)
> 
> > It's not clear to me. |nsIncreaseFontSizeCommand::DoCommand()| just calls
> > |nsIHTMLEditor::IncreaseFontSize()| without checking if it's enabled. What's
> > the point you don't think they are same?
> 
> You're correct, |nsIncreaseFontSizeCommand::DoCommand| doesn't check the enabled/
> disabled state of the command, my bad. But you originally wanted to replace this
> API by an |execCommand()| call and execCommand *DOES* verify if the command is
> enabled or disabled in
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#3231

Thanks, I couldn't find this.

> Furthermore, I am still saying switching from a direct, simple, easy to read,
> easy to maintain API on the editor to an awful, long, complex, error-prone
> call through the commandDispatcher, the controller for a command, the
> command itself is overkill. Such a call is basically unworkable so all users and embedders
> will recreate a simpler API. In other terms, the proposed change is of little value.

I don't think that using commandDispatcher is so bad because you don't need to cache the controller. If it's too bad, I think that there should be standardized editor API.


Anyway, I don't agree to change actual function with this kind of change but I really have a question, what's the case do you need to change the font sizes even when the commands are disabled?

Comment 16

[Makoto Kato [:m_kato]]

I would like to know technical reason why you cannot replace it with another way...  If API cannot replace with another way, I don't change it like bug 1320637 even if no addons use it.

To write simply code and readable code, you could create helper libraries and functions for your editor code.  I  think that currently nsIEditor and favor are too complex and non-readable APIs (Since you wrote original and read it during a lot of years, you might understand nsIEditor, nsIHTMLEditor and etc), so we want to change it to simply.  But "Please don't do that" means we cannot change everything even if clean up...

Comment 17

[Jorg K (CEST = GMT+2)]

FWIW: I was led to believe that [de|in]creaseFontSize() was going to be removed altogether, see bug 769604 comment #0. So I removed their use in Thunderbird in that bug. One argument for the removal was that those <small> and <big> tags don't give a defined font size. Anyway, BlueGriffon is in a different position.

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Jorg K (GMT+1) from comment #17)

> Anyway, BlueGriffon is in a different position.

FWIW, I did remove the cmd_increaseFontSize and cmd_decreaseFontSize buttons
from BlueGriffon, users did complain and they're back in forthcoming public
version. I also removed the buttons corresponding to cmd_fontColor, cmd_backgroundColor
and cmd_highlight and users complain so much and so often they will also be back.

Html cleanliness, standards conformance and users' skills and expectation
are different beasts...

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Makoto Kato [:m_kato] from comment #16)
> I would like to know technical reason why you cannot replace it with another
> way...  If API cannot replace with another way, I don't change it like bug
> 1320637 even if no addons use it.
> 
> To write simply code and readable code, you could create helper libraries
> and functions for your editor code.  I  think that currently nsIEditor and
> favor are too complex and non-readable APIs (Since you wrote original and
> read it during a lot of years, you might understand nsIEditor, nsIHTMLEditor
> and etc), so we want to change it to simply.  But "Please don't do that"
> means we cannot change everything even if clean up...

I think I gave reasons why I am saying "please don't do that". I am not
objecting to keep a status-quo; I am objecting because this makes no real
sense. It *drastically* complexifies embedding, it will *drastically*
complexify technical documentation, it will almost certainly force all
users and embedders to rewrite the same code to hide that complexity,
and the proposed |execCommand()| call is not a replacement since it does
nothing if the command is disabled. The only plus is a "minus one" on the
number of scriptable methods on nsIHTMLEditor. Sorry, that does not balance
the hassle described in lines above.

Comment 20

[Jorg K (CEST = GMT+2)]

I squashed all user complaints (of which there were very few), that stuff is positively not coming back to TB ;-)

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Jorg K (GMT+1) from comment #20)
> I squashed all user complaints (of which there were very few), that stuff is
> positively not coming back to TB ;-)

Certainly easier for TB than for BlueGriffon :-)

Comment 22

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

Let me sort out the reasons why you think that this change doesn't make sense:

1. Using a command instead of using a method of nsIHTMLEditor makes the caller ugly.
2. Needs some cost to rewrite the callers in embedders.
3. Using execCommand() is different from a call of a method of nsIHTMLEditor because it checks if the command is enabled.
4. Making them non-scriptable doesn't have enough merit to Gecko itself.


I still think that #1 doesn't make sense.  Using command to modify editor contents is a standard API. And developers can wrap/hide the command dispatcher with a function.  (Similar thing is usually used by native applications to hide platform API calls.) Then, any complicated logic which needs to use the command doesn't become messier than using nsIHTMLEditor's method.  So, if you think that using command to modify the editor contents doesn't make sense, you should suggest new API to Editing API WG or somewhere.

I'm sorry about the #2. But please check the answer for #4.

#3 shouldn't be a problem because nsIHTMLEditor is available only in chrome context. So, emmbedder app can access command controller directly and can cache the controller for simpler implementation.

I disagree with #4. It's too bad for us to keep multiple paths to execute a job. Imagine, somebody could add some code into command class, rather than in nsIHTMLEditor's method.  Of course, it shouldn't be done.  However, for preventing such regression, we need to create automated tests for each path but of course, it really wastes our time.  And this bug is a step of making editor implementation simpler.  In my idea, we should get rid of nsI*Editor's methods as far as possible and our internal code should access editor without the interfaces. I.e., command classes should access HTMLEditor directly instead of using nsIHTMLEditor interface. So, making them non-scriptable is a start to get big merit.  Please note that I do NOT mean that we'll cut available features for embedders.


If I misunderstand something, let me know.

# And I'm still not sure the cases which you need to modify contents with disabled commands.
## So, if it's important, we should add automated test for such case.  Although, I don't know how to disable each command from script.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Daniel Glazman (:glazou) from comment #19)
> 
> I think I gave reasons why I am saying "please don't do that". I am not
> objecting to keep a status-quo; I am objecting because this makes no real
> sense. It *drastically* complexifies embedding, it will *drastically*
> complexify technical documentation, it will almost certainly force all

You know, before Gecko 2.0 (Firefox 4), nsIEditor and nsIHTMLEditor were frozen interface.  But :bsmedberg decided that all XPCOM interface was unfrozen from Gecko 2.0.  So XPCOM interfaces are often changed.  (And, some people wants removing XPCOM at finally)

So, as embedding, we don't have stable APIs unfortunately, you know.  You should discuss with :myk for stable embedding APIs.  He is owner for embedding.

And I think that our editor API should be replaced with standardized API as possible. All web developer can use it on cross-browser.

> users and embedders to rewrite the same code to hide that complexity,
> and the proposed |execCommand()| call is not a replacement since it does
> nothing if the command is disabled. The only plus is a "minus one" on the
> number of scriptable methods on nsIHTMLEditor. Sorry, that does not balance
> the hassle described in lines above.

Before writing a patch, I check comm-central and all addon code whether they uses it.  In this case, all addon doesn't call this methods.  And, I cannot check your application code since it isn't Mozilla.  So, is it hosted on github? (or not public since it has paid version?).  I would like to know whether you use any methods of nsIEditor / nsIHTMLEditor to clean up.

Comment 24

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Makoto Kato [:m_kato] from comment #23)

> You know, before Gecko 2.0 (Firefox 4), nsIEditor and nsIHTMLEditor were
> frozen interface.  But :bsmedberg decided that all XPCOM interface was
> unfrozen from Gecko 2.0.  So XPCOM interfaces are often changed.  (And, some
> people wants removing XPCOM at finally)

I don't think you have to tell me about that, I was there.

> So, as embedding, we don't have stable APIs unfortunately, you know.  You
> should discuss with :myk for stable embedding APIs.  He is owner for
> embedding.
> 
> And I think that our editor API should be replaced with standardized API as
> possible. All web developer can use it on cross-browser.

Wait. Having APIs reachable from web content does NOT mean that embed-level
APIs have to be dropped.

> Before writing a patch, I check comm-central and all addon code whether they
> uses it.  In this case, all addon doesn't call this methods.  And, I cannot
> check your application code since it isn't Mozilla.  So, is it hosted on
> github? (or not public since it has paid version?).  I would like to know
> whether you use any methods of nsIEditor / nsIHTMLEditor to clean up.

Yes, it has always been public since the free version is tri-licensed. Just as
Nvu before.

  https://github.com/therealglazou/bluegriffon/tree/bg2

I'll compile that information or nsIEditor and nsIHTMLEditor in BlueGriffon for you.

Comment 25

[Makoto Kato [:m_kato]]

(In reply to Daniel Glazman (:glazou) from comment #24)
> (In reply to Makoto Kato [:m_kato] from comment #23)
> 
> > You know, before Gecko 2.0 (Firefox 4), nsIEditor and nsIHTMLEditor were
> > frozen interface.  But :bsmedberg decided that all XPCOM interface was
> > unfrozen from Gecko 2.0.  So XPCOM interfaces are often changed.  (And, some
> > people wants removing XPCOM at finally)
> 
> I don't think you have to tell me about that, I was there.
> 
> > So, as embedding, we don't have stable APIs unfortunately, you know.  You
> > should discuss with :myk for stable embedding APIs.  He is owner for
> > embedding.
> > 
> > And I think that our editor API should be replaced with standardized API as
> > possible. All web developer can use it on cross-browser.
> 
> Wait. Having APIs reachable from web content does NOT mean that embed-level
> APIs have to be dropped.

Why? if standardized API (ex. execCommands or others) matches your usages, I think that you will use it without this objection...

Additional, unfortunately, Gecko developers might remove a lot of XPCOM interfaces because extension architecture is moving to WebExtension.  If you have an objection that a kind of XPCOM interface is changed, like this bug, please comment the objection to current dev.platfrom thread (Tracking bug for removals after XPCOM extensions are no more?, https://groups.google.com/forum/#!topic/mozilla.dev.platform/PChACMh1W8Y).  :bsmedberg says "if our product no longer needs some functionality (and it's "API"), let's remove it. Quickly and ruthlessly. We need to actively work to maintain less code.".  If no objection about it, we can remove any methods like this bug and we don't have any reason that this bug is rejected.

> > Before writing a patch, I check comm-central and all addon code whether they
> > uses it.  In this case, all addon doesn't call this methods.  And, I cannot
> > check your application code since it isn't Mozilla.  So, is it hosted on
> > github? (or not public since it has paid version?).  I would like to know
> > whether you use any methods of nsIEditor / nsIHTMLEditor to clean up.
> 
> Yes, it has always been public since the free version is tri-licensed. Just
> as
> Nvu before.
> 
>   https://github.com/therealglazou/bluegriffon/tree/bg2
> 
> I'll compile that information or nsIEditor and nsIHTMLEditor in BlueGriffon
> for you.

Thank you!!.  Additional, I am happy if you could create the list about nsIHTMLInlineTableEditor, nsIEditorStyleSheets, nsIHTMLAbsPosEditor, nsIPlaintextEditor and nsITableEditor too.