Problem/Motivation

Solving issue #2293589 breaks table row inline buttons, when rows are not high enough to show the button in the middle. Commit: 63a6e44babea30a06f761f06d0d1cf4a4882d49a from Today (3.10.2014)

The main problem is, that there are correct css styles for .dropbutton-multiple, but not .dropbutton-single. Since fixing #2293589, .dropbutton-single inside a table td, will gets displayed wrong (see img).

Proposed resolution

Applay positioning styles for .dropbutton-single to .dropbutton-wrapper, so .dropbutton-single and .dropbutton-multiple will use this formatting styles.

Remaining tasks

(done) create an issue description
(done) find a solution
(done) create a patch)
(done) make screenshots
review and test

User interface changes

Without patch:

After applying patch:

CommentFileSizeAuthor
#14 Screenshot - 10_17_2014 , 1_27_43 PM.png14.95 KBbwinett
#14 Screenshot - 10_17_2014 , 1_27_06 PM.png14.86 KBbwinett
#13 Screenshot - 10_17_2014 , 1_23_14 PM.png28.3 KBbwinett
#13 Screenshot - 10_17_2014 , 1_21_55 PM.png11.1 KBbwinett
#13 Screenshot - 10_17_2014 , 1_18_26 PM.png23.96 KBbwinett
#12 Screenshot - 10_17_2014 , 12_37_43 PM.png24.87 KBbwinett
#12 Screenshot - 10_17_2014 , 12_34_29 PM.png11 KBbwinett
#12 Screenshot - 10_17_2014 , 12_30_26 PM.png26.14 KBbwinett
#11 dropbutton-single--align-problem.patch519 bytesdroplet
#9 dropbutton_single_vertical_align_2349829_9.patch531 bytescjoy
#5 css_alignment_fix-2349829-5.patch397 byteskillerpoke
#4 css_alignment_fix-2349829-1.patch397 byteskillerpoke
#1 Translations for Administrator role | d81.local 2014-10-03 15-58-21.jpg68.84 KBkillerpoke
#1 table_dropbutton_wrapper.patch397 byteskillerpoke
Translations for Administrator role | d81.local 2014-10-03 15-33-23.jpg70.23 KBkillerpoke
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

killerpoke’s picture

killerpoke CreditAttribution: killerpoke commented
Issue summary: View changes
FileSize
table_dropbutton_wrapper.patch397 bytes
Translations for Administrator role | d81.local 2014-10-03 15-58-21.jpg68.84 KB

I just created a patch to fix this problem and updated the issue description.

killerpoke’s picture

killerpoke CreditAttribution: killerpoke commented
Issue summary: View changes
johnennew’s picture

johnennew CreditAttribution: johnennew commented
Status: Needs review » Reviewed & tested by the community

This is a good catch!

I have confirmed the issue.

Steps to reproduce

1. Enable translation modules
2. Goto admin/people/roles/manage/administrator/translate

I have confirmed attached patch corrects the issue
I have confirmed attached patch does not regress #2293589

Just a note - the patch name does not adhere to naming convention - do check docs here: https://www.drupal.org/node/707484

 [short-description]-[issue-number]-[comment-number].patch
 e.g. css_alignment_fix-2349829-1.patch
killerpoke’s picture

killerpoke CreditAttribution: killerpoke commented
FileSize
css_alignment_fix-2349829-1.patch397 bytes
killerpoke’s picture

killerpoke CreditAttribution: killerpoke commented
FileSize
css_alignment_fix-2349829-5.patch397 bytes

Finally figured out, how the patch should be named.

The last submitted patch, 1: table_dropbutton_wrapper.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: css_alignment_fix-2349829-5.patch, failed testing.

The last submitted patch, 4: css_alignment_fix-2349829-1.patch, failed testing.

cjoy’s picture

cjoy CreditAttribution: cjoy commented
FileSize
dropbutton_single_vertical_align_2349829_9.patch531 bytes

Looking at that section of the css, I see:

.js td .dropbutton-multiple {
  display: block;
  min-height: 2em;
  position: relative;
  padding-right: 10em; /* LTR */
  margin-right: 2em; /* LTR */
  max-width: 100%;
}
[dir="rtl"].js td .dropbutton-multiple {
  padding-right: 0;
  margin-right: 0;
  padding-left: 10em;
  margin-left: 2em;
}

You change the first selector to match the wrapper instead of its child, but the RTL override below still applies to the child (thus no longer being an override).
I would suggest is an alternative fix that also works for RTL. Tested on FF, Chrome, Safari (all Mac) and mobile Safari iOS7.

cjoy’s picture

cjoy CreditAttribution: cjoy commented
Assigned: killerpoke » Unassigned
Status: Needs work » Needs review
droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
dropbutton-single--align-problem.patch519 bytes

I added a bit more, @see #2358507: single dropbutton align problem

bwinett’s picture

bwinett CreditAttribution: bwinett commented
FileSize
Screenshot - 10_17_2014 , 12_30_26 PM.png26.14 KB
Screenshot - 10_17_2014 , 12_34_29 PM.png11 KB
Screenshot - 10_17_2014 , 12_37_43 PM.png24.87 KB

I noticed that the issue also shows up on admin/config/regional/language/detection

and on admin/config/people/accounts/translate

and here: admin/config/regional/date-time

I will next make sure they look good after the patch. And I will check pages without the patch that look good, and make sure they still look good after the patch.

bwinett’s picture

bwinett CreditAttribution: bwinett commented
FileSize
Screenshot - 10_17_2014 , 1_18_26 PM.png23.96 KB
Screenshot - 10_17_2014 , 1_21_55 PM.png11.1 KB
Screenshot - 10_17_2014 , 1_23_14 PM.png28.3 KB

With the patch, admin/config/regional/language/detection looks good:

So does admin/config/people/accounts/translate:

As does admin/config/regional/date-time:

bwinett’s picture

bwinett CreditAttribution: bwinett commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screenshot - 10_17_2014 , 1_27_06 PM.png14.86 KB
Screenshot - 10_17_2014 , 1_27_43 PM.png14.95 KB

I checked admin/config/people/accounts/fields before and after the patch, and it good before and after. Before:

And after:

I looked at the code, and it looks like the change is isolated and doesn't introduce any coding standards issues. So setting the status to rtbc.

bwinett’s picture

bwinett CreditAttribution: bwinett commented
Parent issue: » #2293589: Texts are not vertically aligned in listing views

Documenting that #2293589: Texts are not vertically aligned in listing views is this issue's parent.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

I'm super concerned about all the outstanding issues with dropbuttons. I feel we're playing whack-a-mole.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Could really use automated front end testing for this, but appreciate the manual testing that's gone into this issue and the change looks fine.

Committed/pushed to 8.0.x, thanks!

  • catch committed b14664f on 8.0.x 
    Issue #2349829 by killerpoke, droplet, cjoy: Fixed #2293589 breaks table...
YesCT’s picture

YesCT CreditAttribution: YesCT commented

yeah, I wonder if we can look into #2229187: SiteEffect: Automated frontend regression testing as an external service (a simplytest.me kind of thing?)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.