WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#42042 closed defect (bug) (fixed)

Permalink structure tags are sometimes added to the front the of the structure

Reported by: ocean90 Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Permalinks Keywords: has-patch needs-refresh
Focuses: ui, accessibility, administration Cc:

Description

Related: #29872

In some circumstances the tags are not added to the end of the structure.

Steps to reproduce:

  • Set structure to "post name"
  • Save settings
  • Set structure to "custom structure" by clicking into the input field
  • Click the button for a tag
  • Click another button for a tag

Actual: The second tag gets added as the first tag.
Expected: The second tag gets added after the last tag

Attachments (4)

42042.gif (1.3 MB) - added by ocean90 3 months ago.
42042.diff (469 bytes) - added by felipeelia 2 months ago.
42042.2.diff (713 bytes) - added by felipeelia 2 months ago.
ticket-42042.gif (75.2 KB) - added by felipeelia 8 weeks ago.

Download all attachments as: .zip

Change History (18)

@ocean90
3 months ago

@felipeelia
2 months ago

@felipeelia
2 months ago

#1 @felipeelia
2 months ago

  • Keywords has-patch reporter-feedback added; needs-patch removed

As the first patch just sets permalinkStructureFocused to false, this second one gives focus back to input field, so if the user keeps clicking on buttons the slugs are appended where the cursor was, not just at the end of it. I think that makes more sense. Am I right, @ocean90?

This ticket was mentioned in Slack in #core by felipeelia. View the logs.


2 months ago

#3 @peterwilsoncc
8 weeks ago

Reverse appears to happen when adding/removing via the keyboard:

  • Set initial structure to post name
  • Save permalinks
  • Set to custom structure using the keyboard
  • Tab into the custom structure input and place the cursor at the start of the input
  • Tab out and use spacebar to click a button, it will be added to the start of the custom structure
  • Tab and and use spacebar click another permalink, it will be added to the end of the custom structure

Mouse clicks continue to do the reverse.

#4 @felipeelia
8 weeks ago

  • Keywords reporter-feedback removed

Hi @peterwilsoncc,

I can't reproduce that with 42042.2.diff applied. Did you test with it? For me it works like the gif above.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 weeks ago

#6 @afercia
8 weeks ago

  • Focuses accessibility added
  • Keywords needs-refresh added

For accessibility reasons, focus should not be moved programmatically from the currently focused button to the input field. When using only the keyboard or a screen reader, that's not the expected behavior. Users would be confused by the unexpected change of context, not to mention they would be forced to tab back and forth multiple times.

I see and can reproduce the issue with the second tag being inserted in a wrong position, but this shouldn't be fixed introducing an accessibility regression. Hopefully an alternate solution can be found in time for 4.9. If not, I'd say this isn't a critical bug and can be postponed to the next minor release.

#7 follow-up: @afercia
8 weeks ago

I think a new condition needs to be added here:

// Input field never had focus, move selection to end of input.
if ( ! permalinkStructureFocused && 0 === selectionStart && 0 === selectionEnd ) {
	selectionStart = selectionEnd = permalinkStructureValue.length;
}

to check for:

  • if the input fields gets blurred and selectionStart and selectionEnd are 0, then append to the end. cc @swissspidy

#8 in reply to: ↑ 7 @swissspidy
8 weeks ago

That makes sense.

I thought I had something like this in place during development of the feature, maybe I forgot about it.

Replying to afercia:

I think a new condition needs to be added here:

// Input field never had focus, move selection to end of input.
if ( ! permalinkStructureFocused && 0 === selectionStart && 0 === selectionEnd ) {
	selectionStart = selectionEnd = permalinkStructureValue.length;
}

to check for:

  • if the input fields gets blurred and selectionStart and selectionEnd are 0, then append to the end. cc @swissspidy

#9 @felipeelia
8 weeks ago

Hi @afercia and @swissspidy,

In the first patch I've assigned permalinkStructureFocused = false; after that if, what I think does what you're suggesting (since we are dealing with a button click, the text input has lost it focus for sure).

The problem with that is that the second button clicked always append to the end (I don't know if it's really a problem at all). Can we try saving the cursor position in a variable on input blur? If any focus event happen on other element then this text input or the available tags, this var should be nulled. What do you think? I think I can make a patch with this very quickly.

(Sorry, English isn't my first language, feel free to correct me or ask more about this idea.)

Last edited 8 weeks ago by felipeelia (previous) (diff)

#10 follow-up: @afercia
8 weeks ago

Not sure what would be best. Inserting at the end is simpler and sort of "fallback" behavior.

Worth noting I see a different behavior across browsers. I can reproduce this "bug" in Chrome, which returns 0 and 0 for selectionStart and selectionEnd while Firefox appends at the end.

#11 @afercia
8 weeks ago

Note: Firefox changed this behavior about one year ago to follow the HTML spec, see https://bugzilla.mozilla.org/show_bug.cgi?id=1287655
so the initial cursor position in textarea/input is at end of text

#12 in reply to: ↑ 10 @felipeelia
8 weeks ago

Replying to afercia:

Not sure what would be best. Inserting at the end is simpler and sort of "fallback" behavior.

Worth noting I see a different behavior across browsers. I can reproduce this "bug" in Chrome, which returns 0 and 0 for selectionStart and selectionEnd while Firefox appends at the end.

Thanks @afercia. So, do you think the first patch is good enough?

#13 @westonruter
8 weeks ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 42012:

Permalinks: Fix placement of structure tags based on last cursor position.

Props felipeelia, afercia.
See #29872.
Fixes #42042.

#14 @westonruter
8 weeks ago

@swissspidy report from @stino:

Note: When you delete from the text field, the buttons are not deactivated until another button is re-selected.
Note: Sometimes the curser disappears from the text field when removing a tag via deselecting a button. When this happens, the next added tag is added at the start of the string. The cursor disappearing doesn’t happen every time though, but I was able to replicate it 3+ times.

Note: See TracTickets for help on using tickets.