Last modified: 2014-02-04 11:16:39 UTC
CSSJanus is treating box shadows as four-part rules like paddings or borders - where there be pixel values for top, right, bottom, left. But though box shadows can have four parts, the pixel values there refer to x-offset, y-offset, blur radius, and thickness. CSSJanus normally swaps the left and right values, but with a box shadow it winds up swapping the y-offset and thickness, which isn't helpful. What it should be doing instead here is apparently just flipping the sign on the x-offset.
(Similarly for text-shadow.) I'm going to fix this tomorrow. We just need to demand a semicolor or closing brace after the four-part rules, and write another regex or two for the shadows.
I love you.
I148229558e1b.
Merged by Roan.
(In reply to comment #3) > Change-Id: I148229558e1b9a0516e413ffe86007235c3c3ef8 (In reply to comment #4) > Merged by Roan. And reverted. This was previously proposed by Roan himself actually: Change-Id: I5d24c7d8456e271f3a9caf25577acd57586f287a Where Trevor and I agreed that shadows should not be flipped. There was no bug report for it then, hence it not being mentioned here. Sorry for that.
Re-opening as there are two things going on here: * CSSJanus mishandling it (corrupting the data) * CSSJanus not flipping it (keeping it as-is) The first is a bug, the second is a wontfix enhancement. I noticed that the proposed change added a test case where the box-shadow is changed, that's wrong. I don't know how it is currently changing it, but it should be in a state where it isn't changed at all. When I reverted it I thought that that was the current status (it not changing shadows when flipping).
My rationale for this change was implementing shadows on the content container in Vector, see I76d2fd82 by Isarra. Due to implementation details shadows that are almost symmetric are created using non-symmetrical rules. (Of course this can be done by manually creating separate CSS rules for RTL and LTR, but this way would be much nicer.) IMO it would be better to flip by default, and enforce non-flipping where applicable.
Reading the discussion under that gerrit change, it seems like you and Trevor are the only two people opposing it, while it's supported by four people: Daniel, Amir, Matthias, and of course Roan himself. My patchsets adds two new supporters, me and Isarra. That's two against six, including a person who natively speaks an RTL language. I'd appreciate you actually discussing changes - reverts as well - when they're so unobvious. For the record, the self-merged revert is I886a078c. I sumitted a revert of the revert for discussion as I97ee7431.
(In reply to comment #6) > I noticed that the proposed change added a test case where the box-shadow is > changed, that's wrong. I don't know how it is currently changing it, but it > should be in a state where it isn't changed at all. It is possible to have four consecutive numeric values in a box-shadow definition, and this is matched by one of the CSSJanus rules. (It's changed wrong of course, swapping y-offset with blur radius or something like that.)
(CC-ing people who comments on the abandoned changeset - please provide your input. Also CC-ing James, who +1'd the Vector shadows change.)
Out of curiosity, just what was the rationale for not supporting shadow flipping? When the rest of the interface flips as well - and this includes offsets, background positions, and even the entire overall layout - should not shadows do so as well, if they also have offset of their own, to maintain consistency? I would argue that not supporting shadows puts an unfair burden on skin creators - there appear to be very few instances where one would not want a shadow with an x-offset to be flipped along with everything else, and it is much harder to add rules specifically for rtl vs ltr than it is to add a noflip in those cases.
Let's first fix the CSSJanus bug that breaks the 4-part shadow values (mixing blur radius with offset). As for whether or not to flip that's a separate thing. Trevor and I currently agree that it shouldn't be flipped as it doesn't make sense design wise. The shadow isn't read in the text direction and research we did and seen elsewhere showed that the natural fall of light is no different for people who speak an RTL language. We don't mirror theatre, light shows or movie productions either. Having said that, I'd love to hear some rational arguments from you all regarding the why they should be flipped (that is, arguments not related to the development side of things and the bug of it breaking the values).
Per IRC: "* TrevorParscal is ok with flipping shadows assuming it's working properly". Rational arguments have already been made - we flip *everything*, why not shadows? We *could* implement flipping only on request (using a /* @flip */ comment, say), but that would IMO suck more than just flipping it by default and using @noflip where applicable. Let me also quote Amir's comment on the first, abandoned change: (I5d24c7d8) > Yes, shadow should be flipped for RTL, and it should be the default. > It's not just about the light sources - it's about placing other page > components in such a way that the shadow won't interfere with them.
Krinkle, I appreciate that you apparently didn't consider my previous arguments rational, but in light of the lighting concern, I still would argue that not flipping shadows doesn't make sense design-wise. Amir probably makes the main point that if the layout of rest of the interface expects the shadows to be a certain way (and if shadows are a significant part of it chances are it will) then the shadows should be flipped too. But for specific concerns about the light source, if there is a light source prominent enough that the directionality would matter that much with regards to it, chances are there's something wrong with the design in general. Such overly skeuomorphic designs can be cute, but rarely do they work well, especially for websites. That's not to say they can't work, however - but if someone does want to do that, they will need to add rtl-specific/noflip definitions for other pieces of the page like paddings, margins, etc anyway for precisely the reason Amir brought up, so the same can and should be applied to the shadows. Not having shadows flip is an unexpected and unnecessary inconsistency.
(In reply to comment #14) > you apparently didn't consider my previous arguments rational I wasn't referring to your arguments specifically but to the previous comments in general. They aren't irrational, they just weren't talking about justifying or explaining why they should be flipped (as opposed to wanting them to be flipped or explaining they are currently incorrectly flipped). The comments you left that addressed whether we should flip them were quite useful, just like Amir's point is also well-taken and useful! > Amir probably makes the main point that if the layout of rest of the > interface expects the shadows to be a certain way > but in light of the lighting concern, I still would argue that not > flipping shadows doesn't make sense design-wise [..] (and if shadows are a > significant part of it chances are it will) then the shadows should be > flipped too. > > But for specific concerns about the light source, if there is a light source > prominent enough that the directionality would matter that much with regards > to it, chances are there's something wrong with the design in general. > Such overly skeuomorphic designs can be cute, but rarely do they work well I disagree, I think it's quite the opposite. If a layout has a shadow that looks bad when the layout is flipped the shadow is likely overkill. For example a shadow on an element that touches the edges of the window (such as the sidebar) would act wrong if the sidebar is on the right, but the shadow is going to the bottom right still. However the most common and proper use of shadows is where the target element itself is free of the window edges. For example: - Floating elements inside the content area - Text shadow - Modal dialogs In those cases the layout is not malformed or visibly broken (e.g. cut off) if the layout is flipped but not the shadows.
(In reply to comment #15) > If a layout has a shadow that looks bad when the layout is flipped the shadow > is likely overkill. For example a shadow on an element that touches the edges > of the window (such as the sidebar) would act wrong if the sidebar is on the > right, but the shadow is going to the bottom right still. While you might be right (and I'm no designer to discuss it), this is off the topic here. We're talking merely about enabling such a possibility; how it shall be used (or not) in MW core is the subject of different bugs and different changesets. Just because a feature could be misused doesn't mean it may not be implemented. I mean, even templates in wikitext can be misused and cause pages to stop rendering, and yet we allow them (while working on the particular misuses by replacing them with Lua modules). > However the most common and proper use of shadows is where the target element > itself is free of the window edges. For example: > - Floating elements inside the content area > - Text shadow > - Modal dialogs > > In those cases the layout is not malformed or visibly broken (e.g. cut off) > if > the layout is flipped but not the shadows. This be understood the other way as well - flipping the shadows won't break the layout in those cases, either. Krinkle, I'd be happy to have this as a opt-in feature (needing an /* @annotation */ to enable shadow flipping, disabled by default) if you volunteer to implement it. Otherwise, if you have no major objections, I'm asking you to reconsider the revert.
(In reply to comment #16) > (In reply to comment #15) > > If a layout has a shadow that looks bad when the layout is flipped the shadow > > is likely overkill. For example a shadow on an element that touches the edges > > of the window (such as the sidebar) would act wrong if the sidebar is on the > > right, but the shadow is going to the bottom right still. > > While you might be right [this] is off the topic here. We're talking merely > about enabling such a possibility; how it shall be used (or not) in MW core > is the subject of different bugs and different changesets. > > Just because a feature could be misused doesn't mean it may not be > implemented. True, but if the only use case for a feature is a misuse, then it does not make sense to implement it, would you agree? > I mean, even templates in wikitext can be misused and cause pages to stop > rendering, and yet we allow them (while working on the particular misuses by > replacing them with Lua modules). > Off topic. > Krinkle, I'd be happy to have this as a opt-in feature (needing an /* > @annotation */ to enable shadow flipping, disabled by default) I'm pretty sure that adding options for this will solve nothing but allow the developer to make things inconsistent and flip or don't flip based on personal preference which means the same situations will sometimes be flipped and sometimes not based on the developer's decision, that I will not support. > > However the most common and proper use of shadows is where the target element > > itself is free of the window edges. For example: > > - Floating elements inside the content area > > - Text shadow > > - Modal dialogs > > > > In those cases the layout is not malformed or visibly broken (e.g. cut off) > > if the layout is flipped but not the shadows. > > This be understood the other way as well - flipping the shadows won't break > the layout in those cases, either. Indeed, in the most common case the shadows will not look broken both when they're flipped and when they're not flipped. So, then. If the use case I pointed out as misuse is not the use case you push to enable flipping, then what is? When would you enable flipping for a shadow, and why?
I don't know why you consider this a misuse, Krinkle, because it isn't. This bug should be fixed, plain and simple, because that is the user (developer) expectation.
I still tend to be in favor of flipping it. As said before, it would just be consequent. Very complex styling which do not just do shadows but advanced stuff like bright borders on the one side, dark borders on the shadow side, would just look weird if the shadow wouldn't be flipped as well. Also, if the shadow is missuses for layout purposes where the shadow doesn't represent a traditional shadow anymore, the layout would probably break if not mirrored. I have no real opinion on whether this "misuse" is a bad thing or not, I just imagine it could be a valid styling method in certain cases.
https://gerrit.wikimedia.org/r/52611 (Gerrit Change I97ee7431e1a5acb35d594076a88a0f9acf290402) | change ABANDONED [by Hashar]
https://gerrit.wikimedia.org/r/52611 (Gerrit Change I97ee7431e1a5acb35d594076a88a0f9acf290402) | change RESTORED [by Matmarex]
Real world samples (not demonstrations or made up examples) would be useful at this point. Then we can look whether there is another viable solution for the specific use case. Changing around something that could potentially break existing things for a theoretical use case is unlikely to be a satisfying solution.
Real world use cases have been repeatedly provided already, you're just dismissing them as theoretical. And, quite simply, one can't have a practical use case unless the feature it requires is there. And the suggestion that this is going to break some imaginary "existing things" is honestly quite mad (http://xkcd.com/1172/ comes to mind...). "Real world samples would be useful at this point".
The CSSJanus bug here causes the new login/signup forms in rtl (e.g. http://he.wikipedia.org/wiki/Special:UserLogin ) to have a blue bar for input focus instead of a blue shadow. CSSJanus turns box-shadow: #4091ed 0px 0px 5px; into 5px 0 0 #4091ed. I raised the priority and severity of this bug. Krinkle in comment #12 said > Let's first fix the CSSJanus bug that breaks the 4-part shadow values (mixing > blur radius with offset). but I can't see this was ever split into a separate bug. (Amusingly, this blue bar is the exact effect WMF's new Director of UX Jared is proposing for input focus -- psychic code :)
Could somebody please make the executive decision to merge the patch again, given my rationale and the overwhelming support shown by various people who for some reason failed to express this here, but did express it on IRC or on the reverted patch?
(In reply to comment #24) > I raised the priority and severity of this bug. Krinkle in comment #12 said > > Let's first fix the CSSJanus bug that breaks the 4-part shadow values (mixing > > blur radius with offset). > but I can't see this was ever split into a separate bug. I did split the patch now.
With I16cb9e17 merged CSSJanus should stop breaking the shadows (but it still won't flip them properly).
(In reply to comment #27) > With I16cb9e17 merged CSSJanus should stop breaking the shadows (but it still > won't flip them properly). Lowering priority accordingly.
(In reply to comment #27) > With I16cb9e17 merged CSSJanus should stop breaking the shadows (but it still > won't flip them properly). Lowering priority accordingly as that was supposed to be a separate bug.
(In reply to comment #25) > Could somebody please make the executive decision to merge the patch again, > given my rationale and the overwhelming support shown by various people who > for > some reason failed to express this here, but did express it on IRC or on the > reverted patch? I will merge this patch on Thursday September 19 around 21:00 UTC unless someone has objected by then.
I solicited opinions from Trevor and Timo today. I've tried to summarize them below; feel free to correct me if I've misrepresented Trevor said that he believes that flipping of shadows, while it should be implemented, should not be done by default, and he'd like to add a /* @flip */ tag for this case and other cases where we want to support flipping, but not have it be the default action. Timo argued that CSSJanus's primary goal is to make stylesheets that were written for LTR work in RTL as well as possible, minimizing the number of @noflip or @flip or whatever annotations that people have to put in to make things work. That implies that we should only flip things by default if the most common use case calls for that. Timo doesn't believe that flipping makes sense for the most common uses of shadows, but he acknowledges he is neither a designer (at least not primarily) nor a speaker of an RTL language, so he thinks input should be solicited from those two groups. In order to get that kind of input, could people who are familiar with the use cases prepare screenshots of what RTL interfaces look like with and without flipped shadows? That would be helpful especially for talking to people who are knowledgeable about RTL but not necessarily about CSS or specifics of our UI.
(In reply to comment #31) > In order to get that kind of input, could people who are familiar with the > use > cases prepare screenshots of what RTL interfaces look like with and without > flipped shadows? That would be helpful especially for talking to people who > are > knowledgeable about RTL but not necessarily about CSS or specifics of our UI. It's been pointed out that Amir said this was fine back in comment 13, so that's good enough for me to go ahead and merge this.
Change 52611 merged by jenkins-bot: CSSJanus: Support text-shadow and box-shadow flipping https://gerrit.wikimedia.org/r/52611
