Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#33260 closed defect (bug) (fixed)

Customizer: misaligned Widgets reorder button

Reported by: afercia's profile afercia Owned by: obenland's profile obenland
Milestone: 4.3 Priority: lowest
Severity: trivial Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: ui Cc:

Description

The Widgets reorder button is a bit too big and a bit misaligned. As far as I see, in 4.2 looks OK so this is probably a small visual regression.

https://cldup.com/3nvrki0aKZ.png

Attachments (9)

33260.diff (956 bytes) - added by TomHarrigan 9 years ago.
Fix padding on reorder toggle
33260.2.diff (370 bytes) - added by TomHarrigan 9 years ago.
Fix padding on reorder toggle - take 2
33260.3.diff (1.4 KB) - added by TomHarrigan 9 years ago.
Implement fix using button elements, aria tags to be consistent with Menu Customizer
33260.4.diff (1.4 KB) - added by TomHarrigan 9 years ago.
Implement fix using button elements, aria tags to be consistent with Menu Customizer. Fix typo from 33260.3.diff
33260.5.diff (4.6 KB) - added by afercia 9 years ago.
33260.6.diff (4.5 KB) - added by afercia 9 years ago.
33260.7.diff (964 bytes) - added by obenland 9 years ago.
33260.8.patch (384 bytes) - added by mrahmadawais 9 years ago.
Fixes the Reorder Button by addressing the extra padding in span element
Screenshot at Aug 09 07-35-31.png (27.4 KB) - added by mrahmadawais 9 years ago.
Reorder button is now realigned.

Download all attachments as: .zip

Change History (48)

#1 @ocean90
9 years ago

  • Priority changed from normal to lowest
  • Severity changed from normal to trivial

@TomHarrigan
9 years ago

Fix padding on reorder toggle

@TomHarrigan
9 years ago

Fix padding on reorder toggle - take 2

#2 @TomHarrigan
9 years ago

Patch 33260.2.diff removes padding on the .reorder-toggle class. Padding is already applied on the .reorder span contained inside the toggle, and this now gives consistency between the padding of this element and the corresponding reorder button in the Menu Customizer.

http://i.imgur.com/a4c8P8z.png

This ticket was mentioned in Slack in #accessibility by tomharrigan. View the logs.


9 years ago

#4 @TomHarrigan
9 years ago

  • Keywords has-patch added; needs-patch removed

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


9 years ago

#6 follow-up: @obenland
9 years ago

I think I'd rather change .reorder-toggle for widgets to use a button like in Menus. @TomHarrigan, would you mind refreshing your patch and see how big of a change it would be?

#7 in reply to: ↑ 6 @TomHarrigan
9 years ago

Replying to obenland:

I think I'd rather change .reorder-toggle for widgets to use a button like in Menus. @TomHarrigan, would you mind refreshing your patch and see how big of a change it would be?

Sure, @afercia mentioned that in #accessibility, will take a look at it during the hack night ;)

#8 @obenland
9 years ago

  • Owner set to obenland
  • Status changed from new to accepted

#9 @obenland
9 years ago

  • Keywords needs-patch added; has-patch removed

@TomHarrigan, have you had a chance to look into creating a new patch?

#10 @TomHarrigan
9 years ago

@obenland started on, got a lil caught up with work, will finish out and submit today if that works

@TomHarrigan
9 years ago

Implement fix using button elements, aria tags to be consistent with Menu Customizer

@TomHarrigan
9 years ago

Implement fix using button elements, aria tags to be consistent with Menu Customizer. Fix typo from 33260.3.diff

This ticket was mentioned in Slack in #accessibility by tomharrigan. View the logs.


9 years ago

#12 follow-up: @afercia
9 years ago

Notw that it's a button and has a type="button" attribute we should also review the events attached, needs just a click, no keys filtering and no preventDefault() :)

Last edited 9 years ago by afercia (previous) (diff)

#13 in reply to: ↑ 12 ; follow-up: @obenland
9 years ago

Replying to afercia:

Not that it's a button and has a type="button" attribute we should also review the events attached, needs just a click, no keys filtering and no preventDefault() :)

Do you have time to update the patch?

#14 in reply to: ↑ 13 ; follow-up: @TomHarrigan
9 years ago

Replying to obenland:

Replying to afercia:

Not that it's a button and has a type="button" attribute we should also review the events attached, needs just a click, no keys filtering and no preventDefault() :)

Do you have time to update the patch?

type="button" also exists in the button in menu customizer content_template() function, so should it get removed there as well, and have those attached events reviewed too?

#15 in reply to: ↑ 14 ; follow-up: @obenland
9 years ago

Replying to TomHarrigan:

type="button" also exists in the button in menu customizer content_template() function, so should it get removed there as well, and have those attached events reviewed too?

The Menu Customizer should be up to date on that.

#16 in reply to: ↑ 15 @TomHarrigan
9 years ago

Replying to obenland:

The Menu Customizer should be up to date on that.

The patch code was directly 'inspired' by the Menu Customizer code:
http://i.imgur.com/tnbBmof.png

#17 follow-up: @obenland
9 years ago

Oh, he meant that the JS events attached to .reorder-toggle in wp-admin/js/customize-widgets.php should be reviewed. Switching it to a button with type="button" allows us to remove the keyboard event checks.

#18 @TomHarrigan
9 years ago

Ah gotcha, I misread it as meaning that having button of type button was redundant

#19 in reply to: ↑ 17 @afercia
9 years ago

Replying to obenland:

Oh, he meant that the JS events attached to .reorder-toggle in wp-admin/js/customize-widgets.php should be reviewed. Switching it to a button with type="button" allows us to remove the keyboard event checks.

That :) adding type="button" removes any default action (in any browsers) from buttons. So there's no need to prevent a non-existing default action.

@afercia
9 years ago

#20 @afercia
9 years ago

Refreshed patch to review events, dynamically update ARIA attributes and deliver wp.a11y.speak messages. This should pair these two buttons with the Menu Customizer buttons.
Removed moving focus to the first "Move to another area..." control for consistency with the Menu Customizer and also because that control is only visible when there are two or more widget areas.

#21 @obenland
9 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

We're in RC, so we should avoid introducing new strings at this point.
A refreshed patch without that would be pretty close to fixing the issue though.

#22 @afercia
9 years ago

Refreshed. If I'm not wrong, 'Reorder widgets' was the only one new, reverted to just 'Reorder' we should then change it for 4.4. The other 3 ones already exist.

@afercia
9 years ago

#23 @afercia
9 years ago

Oh well I was wrong, there are also the strings added in the initial patches. What should we do with those ones? At this point maybe we should fix just the styling.

This ticket was mentioned in Slack in #core-customize by afercia. View the logs.


9 years ago

#25 @helen
9 years ago

Yes, at this point we should just fix the styling, and leave the rest for 4.4 (early).

#26 @ocean90
9 years ago

  • Keywords commit added; needs-refresh removed

I agree, 33260.2.diff is OK to close this ticket for 4.3.

#27 @obenland
9 years ago

Thanks, will do

@obenland
9 years ago

#28 @obenland
9 years ago

@helen, @ocean90, rather than removing it from widgets, what do you think about limiting menu styles to menus?

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


9 years ago

#30 follow-up: @ocean90
9 years ago

33260.7.diff means, that we have to add .customize-control-sidebar_widgets to all three rules if we switch this to a <button> element.
But for 4.3 this works too. In 4.4 we should switch it to a <button> element and move it into customize-controls.css.

#31 in reply to: ↑ 30 ; follow-up: @obenland
9 years ago

Replying to ocean90:

In 4.4 we should switch it to a <button> element and move it into customize-controls.css.

This.

#32 @mrahmadawais
9 years ago

@obenland @ocean90 I am up for this.
Actually, the real issue seems to be the .reorder-toggle padding in customize-widgets(-rtl).css file which is adding wrong padding.

I removed those paddings by making them 0 and voila it is perfectly aligned with the other span button. Let me add the patch.

P.S. I missed one comment in the discussion last time. But looks like, since we are not opting to make this span element a button we might as well just make that padding's value 0 for 4.3 release.

Last edited 9 years ago by mrahmadawais (previous) (diff)

@mrahmadawais
9 years ago

Fixes the Reorder Button by addressing the extra padding in span element

@mrahmadawais
9 years ago

Reorder button is now realigned.

#33 in reply to: ↑ 31 @mrahmadawais
9 years ago

Replying to obenland:

Replying to ocean90:

In 4.4 we should switch it to a <button> element and move it into customize-controls.css.

This.

That's true, it will make the overall workflow better.

Kindly, look at the patch I submitted, it addresses the padding issue for the time being.

#34 @obenland
9 years ago

Hi mrahmadawais, thanks for your patch!

TomHarrigan orriginally suggested it too in 33260.2.diff, but it's not widgets that do something wrong here, the menu styles are not properly name-spaced. 33260.7.diff fixes that.

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


9 years ago

#36 follow-up: @helen
9 years ago

33260.7.diff seems good for 4.3, let's open a new ticket for 4.4 if there isn't one already. Needs second committer review but looks good to me.

#37 @azaozz
9 years ago

33260.7.diff looks good.

Version 0, edited 9 years ago by azaozz (next)

#38 in reply to: ↑ 36 @obenland
9 years ago

Replying to helen:

let's open a new ticket for 4.4 if there isn't one already.

#33327

#39 @obenland
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 33604:

Customize: Limit styles for menu's reorder button to the nav_menu control.

This prevents it from bleeding into other controls, misaligning their buttons.
Selectors use a class now too, which is preferable over ids.

Fixes #33260.

Note: See TracTickets for help on using tickets.