Skip to content

UI: Refactor PostTitle for easier select, deselect - #5422

Merged
aduth merged 5 commits into
masterfrom
update/title-padding
Mar 7, 2018
Merged

aduth merged 5 commits into
masterfrom
update/title-padding

Conversation

@aduth

@aduth aduth commented Mar 5, 2018

Copy link
Copy Markdown
Member

This pull request seeks to refactor the PostTitle component. Initially I started this changeset as merely moving padding from the container element to the textarea to allow it to be more clickable by a few pixels, but quickly discovered this is a complex component and, as far as I can tell until directed otherwise, needlessly so.

The user-impacting results of these changes are:

  • Clicking anywhere within the hovered border of the title field selects it (in master, there's 14px of dead zone padding)
  • Clicking anywhere immediately outside the border of the title field unselects it
    • While still preserving clickability of permalink element†
      • †With slight difference in that if clickable target is not inherently focusable, it will be dismissed (e.g. clicking link or "Copy" works fine, but clicking "Permalink:" text will dismiss)

The technical results of these changes are:

  • Removing an unnecessary div wrapper
  • Reusing withFocusOutside to simplify focus outside behaviors and normalize across browsers

This should have been multiple separate commits, but it unintentionally turned into a rabbit hole of changes 😬

Testing instructions:

Verify that you can easily select and unselect the title field.

Verify that you can still click the permalink options for a saved post upon selecting the title.

Verify that the permalink options are dismissed after typing within the field after selected.

@aduth aduth added the General Interface Parts of the UI which don't fall neatly under other labels. label Mar 5, 2018
@aduth

aduth commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

Approximate representation of "dead zone" for clicking into and out of the title, in master (just showing the left-hand side):

master dead zone

@youknowriad

Copy link
Copy Markdown
Contributor

I recall the complexity being needed because when we used to click on the "copy" button the title got unselected. I don't know if it's still the case with this PR, I'll test tomorrow.

@aduth

aduth commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

@youknowriad Yeah, I suspect (or hope at least) that the addition of withFocusOutside since then has the benefit of smoothing out some of these complexities.

@youknowriad

Copy link
Copy Markdown
Contributor

Yeah, it looks like this branch is suffering the issue I'm describing above: you can't click the "copy" button.

@aduth

aduth commented Mar 6, 2018

Copy link
Copy Markdown
Member Author

Hmm, another Firefox-specific focus issue 😞 It works in Chrome, I had tested it. Will debug Firefox now...

@aduth

aduth commented Mar 6, 2018

Copy link
Copy Markdown
Member Author

Actually, I imagine it's the same issue we've been combatting for a while, which is that clicks on buttons don't trigger focus in Firefox, so withFocusOutside isn't capturing the refocus that occurs.

@aduth

aduth commented Mar 6, 2018

Copy link
Copy Markdown
Member Author

The changes in 43f7b85 add some normalization to withFocusOutside. It's nice that this is abstracted up to a common component which is likely to suffer this inconsistency elsewhere, but I'd still love to see some sort of "shim" to normalize this behavior for all buttons on a page.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
https://gist.github.com/cvrebert/68659d0333a578d75372

@aduth

aduth commented Mar 7, 2018

Copy link
Copy Markdown
Member Author

Appears that Safari suffers the same issue for links, so it's not possible to click the permalink.

Obvious solution would be to include this in the isFocusNormalizedButton function here.

Alternatively, I'm wondering if it makes sense for us to apply tabIndex={ -1 } to the wrapper applied by withFocusOutside. Technically in all of the above cases, focus has indeed left, so it's a true representation of withFocusOutside, but focus leaves as a result of clicking in an area that is still part of the same children of withFocusOutside.

@aduth
aduth force-pushed the update/title-padding branch from 67f1c02 to 5487949 Compare March 7, 2018 16:18
aduth added 5 commits March 7, 2018 11:27
Nature of withFocusOutside is that there'll be a short delay between focus being lost from textarea and handleFocusOutside being called, causing the permalink display and title selected effects from dismissing out of sync. While we _can_ style based on textarea focus, we'd rather ensure sync. Ideally, there'd not be a delay from `withFocusOutside`.

Also, we spend a fair bit of effort overriding styles from main.scss which shouldn't be applied to the title in the first place. Why do we assume there's a textarea element? This is abstracted into `Textarea` component, but we have no guarantee that's the tag rendered.
Safari does not count clicks on anchors as focus
@aduth
aduth force-pushed the update/title-padding branch from 5487949 to 7f7f8b3 Compare March 7, 2018 16:27
@aduth
aduth merged commit fe83f67 into master Mar 7, 2018
@aduth
aduth deleted the update/title-padding branch March 7, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

General Interface Parts of the UI which don't fall neatly under other labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants