Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Styling inline editor nested inside table causes styles leaking #2403

Closed
markussc opened this issue Sep 12, 2018 · 22 comments
Closed

Styling inline editor nested inside table causes styles leaking #2403

markussc opened this issue Sep 12, 2018 · 22 comments
Assignees
Labels
plugin:tableselection The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Milestone

Comments

@markussc
Copy link

markussc commented Sep 12, 2018

Type of report

Bug

Provide detailed reproduction steps (if any)

  1. Open http://jsfiddle.net/r9bfgwsk.
  2. Type foo inside a table cell.
  3. Select whole text ( with text selection, not table selection).
  4. Mark selected text bold.

Expected result

The selected text is bold.

Actual result

The table cell gets a grey background. The css-class "cke_table-faked-selection" is added to the table's td-element

Other details

  • Browser: Firefox 62.0
  • OS: Windows 10
  • CKEditor version: 4.10.0
  • Installed CKEditor plugins: sharedspace,lite,undo
@jacekbogdanski jacekbogdanski self-assigned this Sep 12, 2018
@jacekbogdanski
Copy link
Member

Hello,

I tried to reproduce your issue without success. Could you check if you can still reproduce it with clean CKEditor installation (without external plugins)? You could use https://ckeditor.com/latest/samples/ to check if it works correctly.

@markussc
Copy link
Author

markussc commented Sep 12, 2018

It's a bit weird. When I download https://github.com/ckeditor/ckeditor-releases/archive/4.10.1/4.10.1.zip , the file ckeditor.js contains at lines 1056 / 1057:
if(p.active&&c&&p.first.getAscendant("table").equals(c.getAscendant("table"))){g=e(p.first,c);if(!p.dirty&&1===g.length&&!a(f.data.getTarget()))return d(b,"mouseup"===f.name);p.dirty=!0;p.last=c;l(b,g)}}function h(a){var b=(a=a.editor||a.sender.editor)&&a.getSelection(),c=b&&b.getRanges()||[],e;if(b&&(d(a),b.isInTable()&&b.isFake)){1===c.length&&c[0]._getTableElement()&&c[0]._getTableElement().is("table")&&(e=c[0]._getTableElement()); e=q(b,e);a.fire("lockSnapshot");for(b=0;b<e.length;b++)e[b].addClass("cke_table-faked-selection");

The interesting part is the last statement, i.e. adding class "cke_table-faked-selection". This will be added to all ascendant table elements. In my case also the surrounding table. I can fix it if prepending a check like:
if (!e[b].parent().hasClass("cke_editable")) {continue;}
I don't find this piece of code in the src/ directory anywhere. Where does it come from in the released zip ?

@markussc
Copy link
Author

Oh, I just realize that the link to the release-zip points to another github repository (ckeditor-releases instead of ckeditor-dev). What is the relation between the two? Do the releases published in ckeditor-releases not base on the code in ckeditor-dev ? confused..

@jacekbogdanski
Copy link
Member

As I understand your issue is about creating table selection when selecting whole text and applying style. If so, this issue is not limited to forms - exists for any element inside table cell:

tableselection

I agree that selection shouldn't be changed when styling text selection inside a table cell. Please, let us know if I'm wrong about the issue.

About our repositories structure - ckeditor-dev is used for development purposes and may be unstable. It's probably best (especially for production) if you stick with ckeditor-releases. However if you fine with some possible issues, ckeditor-dev gives you bleeding edge CKEditor version.

@jacekbogdanski jacekbogdanski added type:bug A bug. status:confirmed An issue confirmed by the development team. plugin:tableselection The plugin which probably causes the issue. and removed status:pending labels Sep 14, 2018
@jacekbogdanski jacekbogdanski changed the title CSS class added to surrounding table element (cke_table-faked-selection) Styling table cell text causes selecting whole cell Sep 14, 2018
@jacekbogdanski
Copy link
Member

I updated the title and your reproduction steps to better reflect the issue.

@sasgeoff
Copy link

We are attempting to upgrade our product to 4.10.1 and discovered this problem. This not only changes the background color of the td but also prevents the highlighting of selected text, although the selection does take place. Also, the background color we are seeing is darkgray, taken from tableselection.css:

.cke_table-faked-selection {
background: darkgray !important;
color: black;
}

Is there a workaround for this without editing the source?

@sasgeoff
Copy link

Also, the darkgray background color remains even after selecting other elements.

@mlewand mlewand added this to the Backlog milestone Sep 25, 2018
@markussc
Copy link
Author

What @jacekbogdanski added in the video above is not exactly what we are experiencing. We have a table where multiple ckeditor fields (and other content) are contained. When we mark text within a ckeditor field and make it bold (for example), the whole td-table element where the ckeditor field is in becomes gray. This means the grey background is not in the ckeditor field, but around it. This false behaviour comes from the code snipped cited above. See my screenshot:
grafik

@jacekbogdanski
Copy link
Member

jacekbogdanski commented Sep 26, 2018

@sasgeoff broken selection responsiveness after toggling styles may be caused by #2419. However, I don't have an issue with the remaining cell background color on selection change. Can you provide more details e.g. OS and browser? Are you able to reproduce the issue using https://ckeditor.com/latest/samples/ ? Some GIF image would be very helpful thus selection may be placed differently and sometimes it's hard to spot the issue.

@markussc I'm not able to reproduce the issue with a grey background - are you sure that it's not your custom CSS styles for .cke_table-faked-selection class? Can you reproduce the same issue using https://ckeditor.com/latest/samples/ ?

@sasgeoff
Copy link

I have reproduced this issue in jsfiddle:
http://jsfiddle.net/r9bfgwsk

OS: Windows 10, browser: Chrome, CKEditor version: 4.10.1

@markussc
Copy link
Author

Thanks @sasgeoff , this corresponds exactly to my issue.

@jacekbogdanski
Copy link
Member

Thanks! Now we are on the same page. I can confirm the issue and I will update the issue description accordingly to new information. Also, I found that the issue with the blue background caused by fake selection is already reported here: #2357, so there is no need for extracting it into a separate ticket.

@jacekbogdanski jacekbogdanski changed the title Styling table cell text causes selecting whole cell Styling table cell text inside editable field causes faked selection Sep 28, 2018
@sasgeoff
Copy link

sasgeoff commented Oct 1, 2018

Will a fix be in the next release (4.10.2)? Do you know the target date for the next release?

markussc added a commit to markussc/ckeditor-dev that referenced this issue Oct 4, 2018
@markussc
Copy link
Author

markussc commented Oct 4, 2018

@jacekbogdanski : just created a simple fix for it (#2459). Maybe you don't want to merge it like this, but it should give an idea where to start with.

@jacekbogdanski jacekbogdanski self-assigned this Oct 11, 2018
@jacekbogdanski
Copy link
Member

@sasgeoff I'm not able to give you information when the issue will be fixed - currently, we have a lot of work on the table. However, you can give this issue thumbs up 👍 so we will see your interest in fixing it.

@markussc thanks for your input!

@jacekbogdanski jacekbogdanski changed the title Styling table cell text inside editable field causes faked selection Styling inline editor nested inside table causes styles leaking Oct 12, 2018
@markussc
Copy link
Author

FYI: Bug was introduced in 4.7.0 (last working version is 4.6.2)

@sasgeoff
Copy link

sasgeoff commented Nov 8, 2018

My management is asking me to escalate this issue. They see this as a block that is preventing us from obtaining some very important security updates. This issue breaks our product and so prevents us from moving to the latest version. Please consider this fix with the highest priority. We appreciate your focus on this issue as soon as possible.

@mlewand mlewand added the support An issue reported by a commercially licensed client. label Nov 21, 2018
@mlewand mlewand modified the milestones: Backlog, 4.11.2 Nov 21, 2018
@Comandeer
Copy link
Member

Fixed in 4.11.2.

@mlewand mlewand added the target:minor Any docs related issue that can be merged into a master or major branch. label Dec 5, 2018
@sasgeoff
Copy link

sasgeoff commented Jan 7, 2019

Is there a planned date for 4.11.2?

@lslowikowska
Copy link

Hey @sasgeoff, the release is planned for January 9. You can always check the release dates in the milestones 🙂

@epchristi
Copy link

epchristi commented Apr 21, 2020

I know this is closed, but I have seen this error reproduce in the latest version. 4.13.1

@f1ames
Copy link
Contributor

f1ames commented Apr 21, 2020

@epchristi if you could provide repro steps, we will be able to investigate this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:tableselection The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

8 participants