-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixed: Context menu is incorrectly positioned when opened with Shift-F10 #1627
Conversation
CHANGES.md
Outdated
@@ -10,6 +10,7 @@ New Features: | |||
* [#1365](https://github.com/ckeditor/ckeditor-dev/issues/1365): [File Browser](https://ckeditor.com/cke4/addon/filebrowser) plugin uses XHR requests by default. | |||
* [#1399](https://github.com/ckeditor/ckeditor-dev/issues/1399): Added possibility to set [`CKEDITOR.config.startupFocus`](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.config-cfg-startupFocus) as `start` or `end` to specify where editor focus should be after initialization. | |||
* [#1441](https://github.com/ckeditor/ckeditor-dev/issues/1441): [Magic Line](https://ckeditor.com/cke4/addon/magicline) line element can now be identified by `data-cke-magic-line="1"` attribute. | |||
* [#1498](https://github.com/ckeditor/ckeditor-dev/issues/1498) : Added new method 'getClientRects()' to CKEDITOR.dom.range, which returns list of rects for each selected element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's more API change than new feature.
core/dom/range.js
Outdated
/** | ||
* Returns array with rectangles of areas covered by ranges. For each DOM element within range there is one rectangle, but it represents only part of element which is within range, not whole element. | ||
* | ||
* @since 4.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update version tag.
core/dom/range.js
Outdated
* Returns array with rectangles of areas covered by ranges. For each DOM element within range there is one rectangle, but it represents only part of element which is within range, not whole element. | ||
* | ||
* @since 4.9.0 | ||
* @returns {Array} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could introduce DOMRect
abstract class to nicely describe, what is returned from this function or CKEDITOR.dom.element.getClientRect
. Then we would have DOMRect[]
instead of Array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted this task to #1866.
core/dom/range.js
Outdated
return rect; | ||
} | ||
|
||
if ( this.document.$.getSelection !== undefined ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if more readable feature detection won't be better (e.g. range.getClientRects
).
|
||
#### Right to left editors: | ||
- The panel shows itself on the left side of selection. | ||
g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it shouldn't be here.
plugins/floatpanel/plugin.js
Outdated
rect, | ||
rtl = this._.dir == 'rtl'; | ||
if ( offsetX === undefined || offsetY === undefined ) { | ||
rectList = this._.editor.getSelection().getRanges()[ 0 ].getClientRects( ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary space inside parentheses.
core/dom/range.js
Outdated
} | ||
|
||
// Fallback helper for browsers that don't support native getClientRects() | ||
function getRect( context ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This polyfill is not correct. It always return one range, but other browsers often return more, eg when selection is multiline.
@@ -0,0 +1,16 @@ | |||
@bender-tags: bug, 4.9.1, 1451 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update version tag.
It seems that this issue is blocked on #1498. |
Issue #1498 was merged to the 4.10.0 release, so this issue is no longer blocked. |
fa97bdf
to
c4c7024
Compare
57d4669
to
8f93758
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think of adding some unit tests, e.g. selecting several places in the editor and checking if position of the menu changes. Tests should also cover case, when there are no valid client rects for selection.
plugins/floatpanel/plugin.js
Outdated
rect, | ||
rtl = this._.dir == 'rtl'; | ||
if ( offsetX === undefined || offsetY === undefined ) { | ||
rectList = this._.editor.getSelection().getRanges()[ 0 ].getClientRects(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to have also fallback for case when getClientRects
return empty array (e.g. due to w3c/csswg-drafts#2514). Because now this code fails for programatically opened context menus:
var editor = CKEDITOR.instances.editor,
range = editor.createRange(),
element = editor.editable().getFirst();
range.setStart( element, CKEDITOR.POSITION_AFTER_START );
range.select();
editor.execCommand( 'contextMenu' );
When getting position of the selection is not possible, falling back to opening menu in position (0,0) seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After recent changes to getClientRects it's highly unlikely to get empty array as rect list, however I will add this fallback in case of future changes to our or native getClientRects.
plugins/floatpanel/plugin.js
Outdated
rectList, | ||
rect, | ||
rtl = this._.dir == 'rtl'; | ||
if ( offsetX === undefined || offsetY === undefined ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comment referencing the issue.
I've moved logic from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugins/contextmenu/plugin.js
Outdated
offsetY = 0, | ||
rect; | ||
|
||
// #1451 when opening context menu via keystroke there is no offsetX and Y passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format for such comments is different:
// Comment (#1451).
plugins/contextmenu/plugin.js
Outdated
rect; | ||
|
||
// #1451 when opening context menu via keystroke there is no offsetX and Y passed. | ||
if ( data.from === 'keystrokeHandler' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this command used in non-keyboard situations? If not, then this conditional seems unnecessary.
Probably it could be used also in programmatic way, but the question is: what are drawbacks of disabling positioning menu in such situation?
Rebased onto latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behaviour of LTR editors is inconsistent with behaviour of RTL editors. In LTR case context menu is pinned to the right edge of the selection. In RTL context menu is also pinned to the right edge of the selection. The only difference is the direction in which menu opens:
IMO in RTL editor context menu should be pinned to the left edge of the selection.
I'm also wondering about behaviour of context menu with multirange selections, e.g. in table selection:
|
||
editor.once( 'panelShow', function( evt ) { | ||
setTimeout( function() { | ||
resume(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | ||
|
||
var tests = { | ||
'Opening context menu with keystroke': function( editor ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests' names should begin with "test".
@@ -0,0 +1,15 @@ | |||
@bender-tags: bug, 4.10, 1451 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update version tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context menu is not correctly positioned for inline editors.
Additionally unit tests are failing when they are launched via Bender Dashboard.
81ae6d2
to
44e8b1d
Compare
It looks correct for me, except for the missing |
…l plugin to contextmenu
…st selected range
… selected element in rtl editors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What changes did you make?
This fix branches from t/1498: pull request.Whenever there is no offsetX or offsetY toolbar will appear next to selection.
Closes #1451