New issue
Advanced search Search tips

Issue 733603 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Selection direction is lost after applying a style change

Project Member Reported by jfernan...@igalia.com, Jun 15 2017

Issue description

Chrome Version: 60.0.3112.20
OS: Linux

What steps will reproduce the problem?
(1) Load the attached test case
(2)
(3)

What is the expected result?

'Anchor' offset is bigger than 'focus' offset, respecting the initial selection direction.

What happens instead?

'Anchor' offset is smaller than 'focus' offset.

 
preserveSelDirection.html
787 bytes View Download
Components: Blink>Editing>Selection
This issue is not reproducible on Firefox or Edge/IE because these browsers are reseting the selection boundaries (at least the values returned by focus/anchor offsets) after applying the style change.

We don't do that, which I guess it's fine. However, I don't see any reason why we should forget about the previous selection direction. 

The attached case applies a new style to a portion of the text, which cause the DOM tree to change so the new styled text has it's own now. Hence, the selection  boundaries are adapted to the new DOM Tree. However, as I said, there is no reason why they should not respect the initial direction.  

Comment 2 by yosin@chromium.org, Jun 16 2017

Labels: Hotlist-Interop
Status: WontFix (was: Untriaged)
Mark "WontFix" because there are not spec about selection after "foreColor" command and
Chrome, and Firefox do same.

Here is a copy of sample: https://jsfiddle.net/7L1w5rf1/ and results:
- Chrome: 0,20
- Edge: 20,0
- Firefox: 0,20
Maybe Safari yields 0,20

The test case is not the one I wanted to use to reproduce the issue. Instead of selecting the whole text, we'd need to select just a portion, so the text node is split and the selection boundaries updated accordingly.  

Attached the correct test case.
preserveSelDirection-1.html
827 bytes View Download
These are the results I've got with the correct test case:

- Chrome: BEFORE: 10,5 AFTER: 5,10
- Edge: BEFORE 10,5 AFTER 5,5
- Firefox: BEFORE 10,5 AFTER 5,5
- Safari: BEFORE 10,5, AFTER 5,10

It's true that the there is nothing in any spec about what to do after "foreColor" command. Actually, the issue is not specific to such command, 
but any that applies a style which would lead to a new DOM node for the 
selected text.

So, I guess WONTFIX make sense, specially consdering that in the case of selecting the whole text we match other browsers' behavior.
Sorry, previous comment had wrong data. This is the correct result: 

- Chrome: BEFORE: 10,5 AFTER: 0,5
- Edge: BEFORE 10,5 AFTER 5,5
- Firefox: BEFORE 10,5 AFTER 5,5
- Safari: BEFORE 10,5, AFTER 5,0
Cc: yosin@chromium.org
So the new data may indicate an interoperability issue, since Safari is avoiding collapsing the selection when applying foreColor, which is correct, but is keeping the selection direction.

@yosin, do you think we should reconsider the WONTFIX ?

Comment 7 by friv...@gmail.com, Jun 20 2017

The specification here is not much of a guidance either way, and is generally unmaintained.

Since there is no interop currently, it seems possible to make a change, assuming of course it is a change for the better.

Of the 3 behaviors listed in https://bugs.chromium.org/p/chromium/issues/detail?id=733603#c5, chrome's seems the least useful. I think we have an opportunity to improve the behavior, so we should take it.
@yosin could you please give your opinion about the last comments ? Thanks.
Owner: jfernan...@igalia.com
Status: Available (was: WontFix)
@yosin, I reopend this issue just in case we have a chance to improve the current behavior of Chrome in this case, at least to match webKit behavior and reduce the interoperability gaps. I think collapsing selection, as FF and Edge seems to be doing, is not correct, but at least I think matching WK is an improvement. 

What do you think ?  

Comment 10 by yosin@chromium.org, Oct 19 2017

Cc: -yosin@chromium.org
Components: -Blink>Editing>Selection Blink>Editing>Command
Thanks for pay attention about this issue and providing correct sample!

Regarding #c5 result, Chrome is different from Safari, it is better to change
Chrome to align with Safari. 

I think this is a kind of regression since some changes in Chrome makes 
differences from Safari.

Sorry, I'm not sure which changes causes this.

Oh, no need to give directions about the changes to be done. It's just htat you marked the issue as WONTFIX and I wanted you to reconsider it :) I think I know what changes has to be done to fix this issue, but I wanted also your opinion whether I should proceed or not. 


I understand that you agree on fixing the issue and will review the CL when it's ready.  
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10d561d9f269ad31d0c7ffa7635cf8e7d57f3078

commit 10d561d9f269ad31d0c7ffa7635cf8e7d57f3078
Author: Javier Fernandez <jfernandez@igalia.com>
Date: Mon Jan 15 21:48:10 2018

Ensure we keep selection direction when applying style changes

For some style changes we update the Selection 'start' and 'end'
boundaries. We must ensure that we keep the same direction used for
create the selection. Hence, if previous Selection was directional,
we must handle the 'start' and 'end' visual Positions according to the
relative order of the 'base' and 'extent' dom Positions in such
previous selection.

The change in the block-style-005.html test is needed because after
this CL, selection direction is preserved from the previous backwards
extent, so we should extend forward instead of starting a new
selection.

Bug:  733603 
Change-Id: I0644bdf41bb60bc90166930c61a0b4fe40d000e2
Reviewed-on: https://chromium-review.googlesource.com/828834
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#529334}
[modify] https://crrev.com/10d561d9f269ad31d0c7ffa7635cf8e7d57f3078/third_party/WebKit/LayoutTests/editing/style/block-style-005.html
[add] https://crrev.com/10d561d9f269ad31d0c7ffa7635cf8e7d57f3078/third_party/WebKit/LayoutTests/editing/style/preserve-selection-direction.html
[modify] https://crrev.com/10d561d9f269ad31d0c7ffa7635cf8e7d57f3078/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp

Status: Fixed (was: Available)
This issue should be FIXED now.

Sign in to add a comment