Closed Bug 1449564 Opened 6 years ago Closed 6 years ago

Disable table/image resizers in default

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files)

Bug 1449564 - part 1: Disable object resizer and inline table editor in default
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1449564 - part 2: Make absolute positioned element editor disabled in default and make it possible to enable it with new command
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1449564 - part 3: Make absolute position editor listen to mouse events at the system event group
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1449564 - part 4: Make users can show Gecko specific editing UIs with new prefs
59 bytes, text/x-review-board-request
m_kato
: review+
Details 
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
Keywords: site-compat
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)
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 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 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
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.
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 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.
(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.
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 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 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 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).
(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 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+
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
Depends on: 1484693
Blocks: 1485244
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)
Regressions: 1551185
Regressions: 1604144
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: