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

Editor: Reset title scroll position on blur #546

Merged
merged 1 commit into from
Nov 24, 2015

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Nov 23, 2015

Fixes #347

This pull request seeks to resolve an issue in Firefox where, after tabbing from the editor title field after having entered a very long title, the field would not reset to the start of the field.

Before:

before

After:

after

Testing instructions:

Verify that the title resets to the show the beginning of the field value when tabbing to the editor. This should behave correctly in Firefox and in your preferred browser.

  1. Navigate to the Calypso post editor
  2. Select a site, if prompted
  3. Enter a long title, enough to scroll past the visible width of the editor
  4. Tab from the field
  5. Note that the beginning of the title field is visible

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 23, 2015
@aduth aduth self-assigned this Nov 23, 2015
@nylen
Copy link
Contributor

nylen commented Nov 23, 2015

Confirming the bug and the fix. 👍

@nylen nylen added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 23, 2015
@@ -66,6 +66,8 @@ export default React.createClass( {
} );

this.onChange( event );

event.target.scrollLeft = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that should really be part of FormTextInput? Feels brittle to be monkeying with a DOM element from a custom element, as you don't really know what event.target is, or if it's the scrollable element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that should really be part of FormTextInput? Feels brittle to be monkeying with a DOM element from a custom element.

That would depend whether we think browser quirks fall under the scope of behaviors to be implemented in the base form components. I'd personally lean towards keeping them as "pure" as possible such that their purpose is primarily aimed at keeping form styling consistent across the app. That said, an alternative view would be to consider them as normalized components, in which this would help keep behavior consistent across browsers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Either way, shouldn't hold the merge.

aduth added a commit that referenced this pull request Nov 24, 2015
…title-scroll

Editor: Reset title scroll position on blur
@aduth aduth merged commit da88fd5 into master Nov 24, 2015
@aduth aduth deleted the fix/347-editor-firefox-reset-title-scroll branch November 24, 2015 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor: the beginning of a title is not visible after tabbing from title to content area
4 participants