item-list class inherited the wrong margin in RTL

Adding style for current page 'admin/people', not for all

I have tested #5 patch but i can not see any effects

assigning to myself during the code sprint in Ghent at XIO

I reviewed the patch (5) and it works as advertised and should be good to go.

I think this needs more investigate how come it looks okay in ltr in HEAD?

@HOG Seems like issues is still there :( Please check screenshot.

RTL margin fixed.

This doesn't look correct. We are removing the LTR styling and only applying RTL styling. Why?

@LewisNyman, thank you for remark. I edited list styles by default & a lists in table for better display.

I'm working this with nicobot in Barcelona 2015.

Patch #18 works. See attached pictures. Ignore test-bot results as it's only css.

The patch contains unrelated changes in sites/example.settings.local.php.

Also, when fixing RTL issues, you should always check that the LTR version doesn't change before/after the patch. The current patch seems to be breaking the LTR version.

Attached a patch with only RTL-related css.

Ok Sokru! I will review the patch, please set the issue to to be reviewed.

BTW, our mentor is jp_stacey.

Hi Sokru,

Patch it's fine! It fixes the issue and it does not add anything else.

Moving to RTBC.

Thanks,
nico

@sokru, @jp_stacey: can you work with another sprinter to add/update screenshots for this issue? We need before and after screenshots. Use Skitch to add arrows so that it's clear what you are pointing out in the screenshot.

As you can see in the screenshots below, there is still some extra margin remaining in the RTL version.

LTR Version (No margins).

RTL Version

Unfortunately, RTL issues can get tricky sometimes. RTL rules can override unrelated LTR rules due to CSS Specificity. Here, [dir="rtl"] .item-list ul wins over td .item-list ul, which has been causing the issue.

Here is a patch with a more proper fix.

I attached a patch, with only setting margin-right: 0, there we won't affect bottom and top margins, which should not be language direction dependent.

Here's the screenshots of changes to [dir="rtl"] .item-list ul element showing the difference for documentation and confirmation.

Before

After

Nevermind my last patch, we should go with Herom's patch in comment #30.

Hi all, it is really helpful when you include an interdiff along with your patch:
https://drupal.org/documentation/git/interdiff

Also, the screenshots need to be embedded in the issue summary (which needs an update to add the template: https://www.drupal.org/issue-summaries)

Just to expand on sokru's comment above (I'm a mentor at Barcelona 2015):

* I agree the patch by herom in #30 is correct.
* The confusion arose because of the interplay of LTR and RTL rules in *both* tables.css and menus-and-lists.css

For the sprint purposes, this patch has been RTBC by people at the mentored sprint, but I don't think herom is here?

This looks like the wrong fix... I think we need to change core/themes/seven/css/components/tables.css instead and do:

[dir="rtl"] td .item-list ul,
td .item-list ul {
  margin: 0;
}

Looking into this issue.

removed css from menus-and-lists.css and add it in core/themes/seven/css/components/tables.css as per feedback in #39.

I have tested patch no #41 on my local machine so its working fine for me.
See the screenshot

Test this patch on drupal-rc1 version

Tested on 8.2.x looks good.

@jordanddisch, could you generate 3 additional screenshots:

- "Before" for LTR
- "After" for LTR
- "Before" for RTL

then make a new comment with all 4 screenshots? We should verify that the change doesn't cause regressions in LTR cases as well.

Testing in 8.2.x, tested switching back between LTR and RTL, looked good.

Tested using latest patch as of May/13/2016 https://www.drupal.org/files/issues/2501957-rtl-41.patch

Before RTL change (text is LTR)

After RTL change (text is RTL)

Before LTR (text is RTL)

After LTR (text is LTR)

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -6,9 +6,6 @@
-[dir="rtl"] .item-list ul {
-  margin: 0.25em 1.5em 0.25em 0;
-}

Why are we removing this code altogether? I think this might be unintentionally breaking something else.

@Cottser i did not found .item-list ul any where as of now.
But on help page there is unordered list but there html rendering structure is different from what we are removing in the patch. (screenshot attached)

Any other who had found the broken UI with removing

-[dir="rtl"] .item-list ul {
-  margin: 0.25em 1.5em 0.25em 0;
-}

Just checked the #41 patch, after applying it shows 1 css property is applied twice. so no need to have this property again.

[dir="rtl"] td .item-list ul,
td .item-list ul {
  margin: 0;
}
/* This is required to win over specificity of [dir="rtl"] .item-list ul */
[dir="rtl"] td .item-list ul {
  margin: 0;
}

This is already there. ([dir="rtl"] td .item-list ul {margin: 0;}). so need of this ([dir="rtl"] td .item-list ul, td .item-list ul) code.
And css is already called from tables.css file.
Screenshot is attached for your reference.

@Maninders I needed to read your comment a few times to understand but this should not be RTBC. What you pointed out is actually that this bug has already been fixed (I manually tested to confirm). We can also see this from the screenshots in #47 as well (no differences).

I ran git log -S '/* This is required to win over specificity of [dir="rtl"] .item-list ul */' and that turned up #2587549: Remaining RTL fixes for Drupal 8.

Unfortunately the only thing that makes sense now is to close this issue, thanks to everyone who worked on it!