Page MenuHomePhabricator

Remove /* @noflip */ Leaflet's CSS fixes
Closed, ResolvedPublic

Description

Now that T148572 has been finished, we should remove all those hacks in lib/mapbox-style-fixes.css and use proper ResourceLoader's config

Event Timeline

Yurik created this task.Oct 19 2016, 10:48 PM
Restricted Application added a project: Discovery. · View Herald TranscriptOct 19 2016, 10:48 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald Transcript
MaxSem moved this task from Needs triage to Maps on the Discovery board.Oct 26 2016, 9:26 PM
Samtar added a subscriber: Samtar.Oct 27 2016, 7:28 AM

I'll happily mentor this for Google-Code-In-2016 - should be nice and easy :D

@Yurik is there any additional information which can be added to this task?

Yurik added a comment.Oct 27 2016, 2:21 PM

Nope, it is an easy one))

Task in GCI: https://codein.withgoogle.com/dashboard/tasks/5185098156605440/

However, the description is a bit short and requires some context (the extension, ResourceLoader, ...), which isn't a big problem, but also not very great :) My feeling is, that a 15 year old student, who contributes the first time, may be a bit overwhelmed with this task as it is :) Maybe you can add some resource where the student can find more information about the involved things in this task? Or, @Samtar, will you be very available to give a starting point for a possible student? Not a guide, but something like a bit context and a "look here, this looks like your starting point"? :D Maybe write it into the task, that the student should feel free to contact you (and probably yurik, too? :D)

@Samtar: ping - can you please reply to the last comment? This task will become available in GCI tomorrow...

Yurik removed a project: Maps.Dec 15 2016, 4:39 AM
divadsn claimed this task.Dec 26 2016, 3:38 PM
divadsn added a subscriber: divadsn.

I will try my best to solve this task :)

Change 329605 had a related patch set uploaded (by Divadsn):
Remove /* @noflip */ Leaflet's CSS fixes

https://gerrit.wikimedia.org/r/329605

Change 329605 merged by jenkins-bot:
Remove /* @noflip */ Leaflet's CSS fixes

https://gerrit.wikimedia.org/r/329605

divadsn closed this task as Resolved.Dec 30 2016, 6:22 PM

@Aklapper can you please approve my task on GCI? :)

As far as I can tell, the map controls now display LTR in a RTL language, which is a regression.
Try: https://en.wikipedia.beta.wmflabs.org/wiki/Map?uselang=he#/map/0

I think we should revert this patch.

Change 330220 had a related patch set uploaded (by JGirault):
Revert "Remove /* @noflip */ Leaflet's CSS fixes"

https://gerrit.wikimedia.org/r/330220

divadsn added a comment.EditedJan 3 2017, 2:43 PM

I cannot reproduce your issue, for me it is displaying right, so this has nothing to do with my patch.

Your example:

My example using a LTR language (Polish):

So I don't see a reason why we should revert my patch, maybe it is something with your browser wrong or with the language itself.

We need someone else to confirm, because that is what I see :

@JGirault, but what is exactly wrong? Because I see no difference, RTL is displaying correctly, so I don't understand your issue.

TheDJ added a subscriber: TheDJ.EditedJan 3 2017, 3:33 PM

@Girault, yes, this also no longer flips the native 'leaflet' controls inside .leaflet-control-container anymore of course.

You could manually flip those using something like:

/* @noflip */
body.rtl .leaflet-left {
    left: initial;
    right: 0;
    .leaflet-control {
        float: right;
        margin-left: initial;
        margin-right: 10px;
    }
}
/* @noflip*/
body.rtl .leaflet-right {
    right: initial;
    left: 0;
    .leaflet-control {
        margin-right: initial;
        margin-left: 10px;
        float: left;
    }
}

Interestingly enough however, this seems to miss certain things like the the scale and attribution elements, which are actually defined in several kartographer specific stylesheets, but don't appear to get flipped either... not sure what that is about... Possibly the noflip of the one RL module get's propagated to all parent's that include it ????? That would be an oversight in T148572.

Long story short, leaflet/mapbox supports being embedded inside an rtl surface, but nothing about leaflet itself will ever do RTL by default. It's support of rtl is therefore 'mixed', and thus it requires us to always apply some adaptations, no matter if we start with a fully flipped surface (now we need to correct for the partial rtl support) or an unflipped surface (now we need to flip all the leaflet UI ourselves).

TheDJ added a comment.Jan 3 2017, 3:36 PM

@divadsn Only the controls 'inside' the surface are wrong. The zoom controls, close button and the attribution.

Compare:

It's not your fault, it's an oversight in the initial analysis of this problem I think.

@divadsn Yes, don't take me wrong, the patch itself is good, but it introduces a regression for our RTL users. Like @TheDJ said, I think we did not anticipate the effects correctly.

You could manually flip those using something like:

/* @noflip */
body.rtl .leaflet-left {
    left: initial;
    right: 0;
    .leaflet-control {
        float: right;
        margin-left: initial;
        margin-right: 10px;
    }
}
/* @noflip*/
body.rtl .leaflet-right {
    right: initial;
    left: 0;
    .leaflet-control {
        margin-right: initial;
        margin-left: 10px;
        float: left;
    }
}

Do you think this is better than the initial mapbox-style-fixes.css ?

@divadsn Only the controls 'inside' the surface are wrong. The zoom controls, close button and the attribution.
Compare:

It's not your fault, it's an oversight in the initial analysis of this problem I think.

Yes, I only noticed that small difference, but I am asking myself if it does matter where these controls are. But I am not a RTL user, so I cannot say anything about that :)

JGirault added a subscriber: Amire80.EditedJan 3 2017, 3:59 PM

@divadsn Only the controls 'inside' the surface are wrong. The zoom controls, close button and the attribution.
Compare:

It's not your fault, it's an oversight in the initial analysis of this problem I think.

Yes, I only noticed that small difference, but I am asking myself if it does matter where these controls are. But I am not a RTL user, so I cannot say anything about that :)

Some time ago we received a request T135935 from a Hebrew user (@Amire80) to flip the controls for consistency.

Maybe we should also apply noflip to RL for kartographer.less, because these controls should be flipped as I see it from this patche here: https://gerrit.wikimedia.org/r/#/c/291100/2/styles/kartographer.less

divadsn added a comment.EditedJan 3 2017, 4:46 PM

I see that someone did revert the change I mentioned before, so we should revert that one or reapply the mentioned above fixes to kartographer.less and it should be ok :)

Change 330220 merged by jenkins-bot:
Revert "Remove /* @noflip */ Leaflet's CSS fixes"

https://gerrit.wikimedia.org/r/330220