Closed Bug 1367460 Opened 7 years ago Closed 7 years ago

Deleting first-images in Medium.com fails (nightly community)

Categories

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

Product:
Core
Component:
DOM: Editor
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed

People

(Reporter: rossgk, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1367460 Shouldn't make focused editing host blurred when moving selection with Selection API
59 bytes, text/x-review-board-request
smaug
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170524030204

Steps to reproduce:

Editor's ("publication owners) in Medium.com get submissions of stories which can be edited. When such a story is opened for editing, and if there is an image as the first element of the submitted story, it cannot be deleted. 
Using Nightly 55.0a1 on MacOSX 10.11.6


Actual results:

I can navigate around, and highlight the image, however I pressing delete, backspace, or any button does not result in the image being deleted. 
Tried it with all add-ons and extensions turned off, and it still doesn't work. 
Tried it with production build of FF and it works okay. 
Verified it works with other browser (Safari).


Expected results:

Should be able to pick an image, or highlight it (eg. drag, or shift-backspace) and press delete to delete it, just like any WYSIWYG editor. 
Granted this may be Medium.com's problem, but thought I'd highlight it here in case it's helpful.
Could you provide a link to reproduce the issue, thanks.
Flags: needinfo?(rossgk)
Unfortunately a link wouldn't help as permissions wouldn't let you test 

I've verified now that the problem exists without requiring the complexities of a editor/writer relationship. It's simply 'stories' in Medium with an image as the first element will not let you delete said image -  thus reproducing is easier:

Need to create (or already be) a Medium user.
  Create a new story
  Make the first element an image. (click plus sign, then select image)
  (May need you to enter a bit of text after the image? Not sure)
  That image cannot be deleted now.


Rgds,
Ross.
Flags: needinfo?(rossgk)
I can reproduce it, it regressed after builds from mid-March. Bisecting right now.
Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6d38ad302429c98115c354d643e81987ecec5d3c&tochange=9a26ed658fdc1d14c6153451e7e45f6ec94292fb

Masayuki Nakano — Bug 1318312
Blocks: 1318312
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
tracking-firefox55: --- → ?
Component: Untriaged → Editor
Ever confirmed: true
Flags: needinfo?(masayuki)
Keywords: regression
Product: Firefox → Core
Although, I don't understand why bug 1318312 caused regression. Because added <img> in medium is wrapped with a <div contenteditable="false">. So, it's impossible to delete unmodifiable element on Gecko.
https://jsfiddle.net/d_toybox/jj661boh/2/

However, on the other browsers, you can delete the image with simple key operation. So, I guess that medium has some hack for us and it needs focus to the <div contenteditable="true"> but it's changed by bug 1318312's fix.

I think that we should support removing <div contenteditable="false"> contents with Delete and Backspace keys. However, it's too risky to do that now.

On the other hand, setting selection to contenteditable="false" contents doesn't cause moving focus from its ancestor editing host. If fixing this incompatibility can fix this bug, it's the best approach.
https://jsfiddle.net/d_toybox/jj661boh/3/
Flags: needinfo?(masayuki)
Should we change the component to Tech Evangelism and contact Medium.com to make modify its code? Maybe it's a safer way to fix it and avoid web compat issues.
(In reply to Loic from comment #6)
> Should we change the component to Tech Evangelism and contact Medium.com to
> make modify its code? Maybe it's a safer way to fix it and avoid web compat
> issues.

I think that if we cannot fix this regression with the last paragraph's approach, I agree with it.
Priority: -- → P2
I understand what's different from Chrome.

Chrome does NOT move focus when focus is moved from an editing host when selection is moved to a non-editable node.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
tracking as a new regression affecting a major site
tracking-firefox55: ? → +
Smaug:

When JS sets selection range to a non-editable node which is outside of a focused editing host, Chrome keeps focus in the editing host but moves selection range to the non-editable node. Then, user cannot edit anything. I don't remember why I tried to steal focus from editing host in such case, though, we should take back the old behavior for now.
FYI: When I run the new tests in Chrome, I still see some difference with Chrome. But most of them are when setting range crosses editing host. I filed a bug to Chromium community to discuss something: https://bugs.chromium.org/p/chromium/issues/detail?id=727609
s/when setting range crosses editing host/when setting range crosses editing host boundaries
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #14)
> Smaug:
> 
> When JS sets selection range to a non-editable node which is outside of a
> focused editing host, Chrome keeps focus in the editing host but moves
> selection range to the non-editable node. Then, user cannot edit anything. I
> don't remember why I tried to steal focus from editing host in such case,
> though, we should take back the old behavior for now.

Isn't Chrome's behavior very weird. Oh well. I guess we can do the same.
Comment on attachment 8872597 [details]
Bug 1367460 Shouldn't make focused editing host blurred when moving selection with Selection API

https://reviewboard.mozilla.org/r/144140/#review147866
Attachment #8872597 - Flags: review?(bugs) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0d668dc55c45
Shouldn't make focused editing host blurred when moving selection with Selection API r=smaug
https://hg.mozilla.org/mozilla-central/rev/0d668dc55c45
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Ross:

I don't use Medium except testing, so, could you verify the fix of the reported behavior and check other functions?
Flags: needinfo?(rossgk)
Thanks to those who contributed on addressing this issue.  I verified the behaviour in rev 55.0a1 (2017-06-02) and it appears to be working well now, and as expected.

Looking good. :)
Flags: needinfo?(rossgk)
Thank you, Ross!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: