Disable table/image resizers in default

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
a year ago
7 days ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete, site-compat})

Trunk
mozilla63
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(4 attachments)

See this spec issue: https://github.com/w3c/editing/issues/171

Only Gecko allows users to resize table/image size with embedded function. However, other browsers do not so. For compatibility with the other browsers, we should disable this at least in default settings.

But perhaps, Thunderbird may need to enable this feature with execCommand.
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Sorry, I missed to catch your replies to my post since it's not delivered by the ML (try to see your replies in Google Group).

My reply for your replies are:

1. Absolutely positioned editor provides grabber to move absolute positioned elements even on Firefox by default.
2. Such Gecko specific editing feature should be disabled by default but allow web apps to enable those features as optional (for backward compatibility).
3. Then, we'll be able to collect actual usage of those minor features with adding telemetry to the nsComposerDocumentCommand.
4. If telemetry indicates nobody doesn't use those features, we can remove them at least from m-c safely (I'm not sure about Thunderbird and Bluegriffon, though).
Jorg, Since we are changing default behavior, do you have any objection for mail edtior?  This features can control by execCommand or XPCOM interface method.
Flags: needinfo?(jorgk)

Comment 17

a year ago
I'll add
  document.execCommand("enableObjectResizing", false, true);
  document.execCommand("enableInlineTableEditing", false, true);
to the TB start-up since we need those resizers to delete rows/columns of tables.
Flags: needinfo?(jorgk)
I assume that all 3 editing UIs are enabled by mail composer and Bluegriffon with XPCOM methods if they need (please do no use execCommand for them since if we can remove them from web, we want to remove (at least) the commands for making simplify the future's execCommand spec).

Comment 19

a year ago
mozreview-review
Comment on attachment 8965540 [details]
Bug 1449564 - part 1: Disable object resizer and inline table editor in default

https://reviewboard.mozilla.org/r/233932/#review240004

::: editor/composer/nsComposerDocumentCommands.cpp:347
(Diff revision 1)
>    NS_ENSURE_ARG_POINTER(aParams);
>    NS_ENSURE_ARG_POINTER(refCon);
>  
> +  // If the result is set to STATE_ALL as bool value, queryCommandState()
> +  // returns the bool value.
> +  // If the result is set to STATE_ATTRIBUTE as CString value,

I wonder that STATE_ATTRIBUTE can has String for cmd_fontFace's parameter....

::: editor/composer/nsComposerDocumentCommands.cpp:450
(Diff revision 1)
>        return NS_ERROR_INVALID_ARG;
>      }
> -
> -    bool enabled;
> -    htmlEditor->GetObjectResizingEnabled(&enabled);
> -    return aParams->SetBooleanValue(STATE_ATTRIBUTE, enabled);
> +    // We returned the result as STATE_ATTRIBUTE with bool value 60 or earlier.
> +    // So, the result was ignored by both nsHTMLDocument::QueryCommandValue()
> +    // and nsHTMLDocument::QueryCommandState().
> +    return aParams->SetBooleanValue(STATE_ALL,

I wonder that what diffrent is the following.
- STATE_ENABLED <-- Bool
- STATE_ALL <-- Bool
- STATE_ANY <-- Bool
- STATE_MIXED <-- Bool
- STATE_BEGIN <-- Bool
- STATE_END <-- Bool
- STATE_ATTRIBUTE <-- CString, String, Bool
- STATE_DATA <-- String, Long and nsISupports

state_all is not good name for return value...
Attachment #8965540 - Flags: review?(m_kato) → review+

Comment 20

a year ago
mozreview-review
Comment on attachment 8965541 [details]
Bug 1449564 - part 2: Make absolute positioned element editor disabled in default and make it possible to enable it with new command

https://reviewboard.mozilla.org/r/233934/#review239920

::: editor/composer/nsComposerDocumentCommands.cpp:334
(Diff revision 1)
>  
>      htmlEditor->EnableInlineTableEditor(enabled);
>      return NS_OK;
>    }
>  
> +  if (!nsCRT::strcmp(aCommandName, "cmd_enableAbsolutePositionEditing")) {

aCommand isn't null, so we use strcmp better instead of nsCRT::strcmp.

But best way is that we don't use C runtime function (ex. bug 1308103) like the following.

nsDependentString commandName(aCommandName);
...
if (commandName.EqualsLiteral("....") {
}

But I will file a bug for it.

::: editor/composer/nsComposerDocumentCommands.cpp:486
(Diff revision 1)
>                                      htmlEditor->IsInlineTableEditorEnabled());
>    }
>  
> +  // cmd_enableAbsolutePositioner is a Gecko specific command,
> +  // "cmd_enableAbsolutePositioner".
> +  if (!nsCRT::strcmp(aCommandName, "cmd_enableAbsolutePositionEditing")) {

Use strcmp better instead of nsCRT::strcmp since aCommandName isn't nullptr.
Attachment #8965541 - Flags: review?(m_kato) → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> I assume that all 3 editing UIs are enabled by mail composer and Bluegriffon
> with XPCOM methods if they need (please do no use execCommand for them since
> if we can remove them from web, we want to remove (at least) the commands
> for making simplify the future's execCommand spec).

Although Edge seems to remove object resizer, we need a telemetry data for usages on real world to remove it.  We might have to add it for enableInlineTableEditing and enableObjectResizing
(Assignee)

Comment 22

a year ago
mozreview-review-reply
Comment on attachment 8965540 [details]
Bug 1449564 - part 1: Disable object resizer and inline table editor in default

https://reviewboard.mozilla.org/r/233932/#review240004

> I wonder that STATE_ATTRIBUTE can has String for cmd_fontFace's parameter....

Isn't it enough if treated as UTF-8 string? (I don't know actual code, though.)

> I wonder that what diffrent is the following.
> - STATE_ENABLED <-- Bool
> - STATE_ALL <-- Bool
> - STATE_ANY <-- Bool
> - STATE_MIXED <-- Bool
> - STATE_BEGIN <-- Bool
> - STATE_END <-- Bool
> - STATE_ATTRIBUTE <-- CString, String, Bool
> - STATE_DATA <-- String, Long and nsISupports
> 
> state_all is not good name for return value...

Yeah, and there is no definition around nsICommandParams. Additionally, between some classes, some different combination from nsComposerDocumentCommand is assumed. Really complicated behavior. I think that it should be sorted out when somebody fixes bug 1450882.
(Assignee)

Comment 23

a year ago
mozreview-review-reply
Comment on attachment 8965541 [details]
Bug 1449564 - part 2: Make absolute positioned element editor disabled in default and make it possible to enable it with new command

https://reviewboard.mozilla.org/r/233934/#review239920

> aCommand isn't null, so we use strcmp better instead of nsCRT::strcmp.
> 
> But best way is that we don't use C runtime function (ex. bug 1308103) like the following.
> 
> nsDependentString commandName(aCommandName);
> ...
> if (commandName.EqualsLiteral("....") {
> }
> 
> But I will file a bug for it.

Perhaps, we should clean up all of nsComposer*Commands.cpp in a bug. Coding styles, name spaces, using non-XPCOM methods as far as possible, this kind of bad code, etc.

Comment 24

a year ago
mozreview-review
Comment on attachment 8965542 [details]
Bug 1449564 - part 3: Make absolute position editor listen to mouse events at the system event group

https://reviewboard.mozilla.org/r/233936/#review240038
Attachment #8965542 - Flags: review?(m_kato) → review+
Thank you, Kato-san for your review.

I put off to land them until Ehsan agrees with this change.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #22)

> Isn't it enough if treated as UTF-8 string? (I don't know actual code,
> though.)

GetCStringValue should has AUTF8String, then remove remove Get/SetStringValue if possible.  (Name isn't good if keeping Get"C"StringValue)
(In reply to Makoto Kato [:m_kato] from comment #21)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> > I assume that all 3 editing UIs are enabled by mail composer and Bluegriffon
> > with XPCOM methods if they need (please do no use execCommand for them since
> > if we can remove them from web, we want to remove (at least) the commands
> > for making simplify the future's execCommand spec).
> 
> Although Edge seems to remove object resizer, we need a telemetry data for
> usages on real world to remove it.  We might have to add it for
> enableInlineTableEditing and enableObjectResizing

Oh, di Edge have it? I added button to toggle the commands but Edge always returns false for queryCommandState() and returns odd string for queryCommandValue() (not "true", "false" etc.) and not showing resizers in my environment.
I added buttons to toggle the command here:
https://d-toybox.com/studio/lib/input_event_viewer.html
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27)

> Oh, di Edge have it? I added button to toggle the commands but Edge always
> returns false for queryCommandState() and returns odd string for
> queryCommandValue() (not "true", "false" etc.) and not showing resizers in
> my environment.

Until IE11, it has resizer that can control by unselectable attribute.  Their implementation doesn't execCommands.
Depends on: 1452538
Keywords: dev-doc-needed
I'm back here now.

According to the result of telemetry, first, we should hide the UIs by default *only* on early Beta and Nightly for now. However, they should be shown by both web developers and users. So, we need to keep supporting execCommand as the patches do, and also we should add prefs to show the features forcibly.

Then, starting from next cycle, 64, we should take the new feature by default in all channels. Then, we can give chance to feedback for testers and web application developers. 

I'll update the patches.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

7 months ago
mozreview-review
Comment on attachment 9001845 [details]
Bug 1449564 - part 4: Make users can show Gecko specific editing UIs with new prefs

https://reviewboard.mozilla.org/r/261698/#review269590


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditor.cpp:3117
(Diff revision 1)
>    // We MUST ONLY load synchronous local files (no @import)
>    // XXXbz Except this will actually try to load remote files
>    // synchronously, of course..
>    RefPtr<StyleSheet> sheet;
>    // Editor override style sheets may want to style Gecko anonymous boxes
>    rv = presShell->GetDocument()->CSSLoader()->

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

  rv = presShell->GetDocument()->CSSLoader()->
  ^

Comment 39

7 months ago
mozreview-review
Comment on attachment 8965540 [details]
Bug 1449564 - part 1: Disable object resizer and inline table editor in default

https://reviewboard.mozilla.org/r/233932/#review269592


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditor.cpp:3068
(Diff revision 2)
>    // We MUST ONLY load synchronous local files (no @import)
>    // XXXbz Except this will actually try to load remote files
>    // synchronously, of course..
>    RefPtr<StyleSheet> sheet;
>    // Editor override style sheets may want to style Gecko anonymous boxes
>    rv = presShell->GetDocument()->CSSLoader()->

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

  rv = presShell->GetDocument()->CSSLoader()->
  ^

Comment 40

7 months ago
mozreview-review
Comment on attachment 8965541 [details]
Bug 1449564 - part 2: Make absolute positioned element editor disabled in default and make it possible to enable it with new command

https://reviewboard.mozilla.org/r/233934/#review269594


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditor.cpp:3068
(Diff revision 2)
>    // We MUST ONLY load synchronous local files (no @import)
>    // XXXbz Except this will actually try to load remote files
>    // synchronously, of course..
>    RefPtr<StyleSheet> sheet;
>    // Editor override style sheets may want to style Gecko anonymous boxes
>    rv = presShell->GetDocument()->CSSLoader()->

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

  rv = presShell->GetDocument()->CSSLoader()->
  ^
Kohei-san:

We won't remove the features at least for now, but we'd like to disable the feature by default, we keep providing existing commands "enableObjectResizing" and "enableInlineTableEditing", and new command "enableAbsolutePositionEditor". Web apps can *restore* the Gecko specific UIs with calls of execCommand(<one of above>, false, true).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #42)
> We won’t remove the features at least for now, but we’d like to disable the
> feature by default

Thanks, I’ve corrected the post.

Comment 48

7 months ago
mozreview-review
Comment on attachment 9001845 [details]
Bug 1449564 - part 4: Make users can show Gecko specific editing UIs with new prefs

https://reviewboard.mozilla.org/r/261698/#review269612
Attachment #9001845 - Flags: review?(m_kato) → review+

Comment 49

7 months ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4e0960a6ffb6
part 1: Disable object resizer and inline table editor in default r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ccb713187e45
part 2: Make absolute positioned element editor disabled in default and make it possible to enable it with new command r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e93a23a4741c
part 3: Make absolute position editor listen to mouse events at the system event group r=m_kato
https://hg.mozilla.org/integration/autoland/rev/541fbb29f21d
part 4: Make users can show Gecko specific editing UIs with new prefs r=m_kato

Comment 50

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e0960a6ffb6
https://hg.mozilla.org/mozilla-central/rev/ccb713187e45
https://hg.mozilla.org/mozilla-central/rev/e93a23a4741c
https://hg.mozilla.org/mozilla-central/rev/541fbb29f21d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1484693
status-firefox61: affected → wontfix
status-firefox62: --- → wontfix
Blocks: 1485244
Duplicate of this bug: 1435646
Depends on: 1486513
No longer depends on: 1486513
I have documented this by:

Updating these command's descriptions to say they are disabled by default in beta/dev edition on https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

Adding a note to the top of https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Editable_content

Adding an entry to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#HTML

I've not updated the compat data for execCommand, or added a note to the Fx63 rel notes, as this update is not available in release yet. I'll do that when the time comes.

Let me know if this is OK. Thanks!
Flags: needinfo?(masayuki)
Keywords: dev-doc-needed → dev-doc-complete
Thank you for updating the documents! Note that only enableAbsolutePositionEditor is introduced since 63.

I'll file a bug to disable them by default in release build with ccing you.
Flags: needinfo?(masayuki)
Depends on: 1490641
You need to log in before you can comment on or make changes to this bug.