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

RichText: Remove focusPosition state #7651

Merged
merged 1 commit into from Jul 3, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Jun 30, 2018

Closes #6640

This pull request seeks to optimize the RichText component to avoid needing to keep a focusPosition state. This state was used exclusively for the positioning of the link popover overlay, and was calculated on every NodeChange event in TinyMCE (which occurs fairly frequently). Its use of Element#getBoundingClientRect would also trigger browser relayout, thus being prone to layout thrashing (source). This revised implementation introduces a new component which is responsible for positioning itself relative the current selection at the time of its own mount. Therefore, it's only calculating its position when the link overlay is actually used.

Testing instructions:

Verify there are no regressions in the display of the link format popover.

This was challenging to implement automated tests for, since JSDOM supports neither window.getComputedStyle nor HTMLElement#offsetParent. End-to-end tests may be wise for link additions, but the specific behavior here is relevant mostly to visual appearance, and we currently have no precedent for visual regression testing (though it may be worth exploring).

if ( this.props.isViewportSmall ) {
let rect;

This comment has been minimized.

@iseulde

iseulde Jul 3, 2018

Member

Why is this needing to be calculated at every node change event? Do we need to scroll every time on that event? Cc @mcsf.

This comment has been minimized.

@aduth

aduth Jul 3, 2018

Member

It almost certainly doesn't need to be. There's definitely room for improvement here, but I considered it outside the scope of the current task.

This comment has been minimized.

@iseulde

iseulde Jul 3, 2018

Member

Sounds good, I like the smaller PRs :)

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

I'm sure there's room for improvement. When we looked in #5769, there was no proper way to detect when the insertion of a character fills the whole width of a text container and forces the caret into a "new line" (a different height in the same container). 'nodechange' is definitely too generic, but per the comment, on nodechange we can work with the finalized TinyMCE instance and scroll to proper position.

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

Perhaps the same kind of reasoning we're seeing in this PR (let's manage this ourselves with our vdom abstraction so as to batch positioning writes and reads) can be applied to the caret-tracking scrolling feature.

@iseulde

This comment has been minimized.

Member

iseulde commented Jul 3, 2018

The only thing I can find here is that the position is not updated after inserting a link without selection:

screen shot 2018-07-03 at 17 55 19

But that certainly better than current master:

screen shot 2018-07-03 at 17 58 23

@iseulde

iseulde approved these changes Jul 3, 2018

@mcsf

mcsf approved these changes Jul 3, 2018

Looks great!

const selection = window.getSelection();
// Get position relative viewport.
const rect = getRectangleFromRange( selection.getRangeAt( 0 ) );

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

Though improbable in the context in which we expect to use this component, should we guard against IndexSizeError by checking selection.rangeCount > 0 first?

This comment has been minimized.

@aduth

aduth Jul 3, 2018

Member

Though improbable in the context in which we expect to use this component, should we guard against IndexSizeError by checking selection.rangeCount > 0 first?

Yep, seems very reasonable. Updated in the rebased 6eed870.

if ( this.props.isViewportSmall ) {
let rect;

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

Perhaps the same kind of reasoning we're seeing in this PR (let's manage this ourselves with our vdom abstraction so as to batch positioning writes and reads) can be applied to the caret-tracking scrolling feature.

* @see https://github.com/tinymce/tinymce/issues/4476
* Handles a horizontal navigation key down event to stop propagation if it
* can be inferred that it will be handled by TinyMCE (notably transitions
* out of an inline boundary node).

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

💯

@aduth aduth merged commit bf09112 into master Jul 3, 2018

2 checks passed

codecov/project 46.61% (-0.1%) compared to 66581eb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the remove/rich-text-focus-position branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment