Page MenuHomePhabricator

[OTRS bug] Disappearing text due to custom styling
Closed, ResolvedPublic8 Estimated Story Points

Description

The first black box in Drew Brees (and many other sports players, as well as many other categories) uses some custom styling.

In mobile web, the box reads "No. 9 - New Orleans Saints"

In mobile-html, the box reads "No. 9 -". You can see that the "New Orleans Saints" text is black, and thus is black-on-black and unreadable.

I believe - but am not sure - this is due to how checkElementForThemeExclusion and notheme works in MobileHTML. The first part of checkElementForThemeExclusion checks if a parent is a theme excluded node, and marks the current node notheme if so. Applying notheme this means Mobile HTML applies the default theming (according to pcs.md, line 476). And so while both halves of the line are marked notheme, because only the th element offers an alternative theme, and the second half renders as black on black.

<th colspan="2" style="[removing to keep brief]" class="notheme">No. 9 – <span class="org notheme">New Orleans Saints</span></th>

By removing notheme from the org span or propagating the same custom theming onto that tag, the text is readable.

Event Timeline

LGoto triaged this task as Low priority.Dec 7 2020, 7:43 PM
LGoto moved this task from Needs Triage to Tracking on the Wikipedia-iOS-App-Backlog board.
AnnaMikla set the point value for this task to 8.Jan 4 2021, 1:13 PM

@MSantos, could you please comment on the next proposal :

The issue described happens when 'notheme' class is added to <span> element which is inside <th> and as the (th) block can be of any color we can not just remove the 'notheme' class.

My proposal is to define if the <th> is dark or light and basing on this to add or not to add the 'notheme' class, so text is always readable. This can be done, for instance, with the help of https://github.com/bgrins/TinyColor#getbrightness library (some other library) or, if it is not acceptable with another method.

So do you think we should go this way ?

Thanks!

@Art.tsymbar thanks for taking a look at it.

I believe there could be a simpler approach, since it's the MobileHtml class that adds the notheme CSS class, it's a performance penalty if we add and check it for removal afterward.

I suggest that we check for this kind of pattern and decide if we add the notheme class or not, this can be done here.

I suggest a allow/disallow list as we do for removing classes that override styles, see https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/618520

The reasoning for this kind of approach is:

  • We take advantage of the current DOM iterator used in MobileHTML lib
  • We avoid 3rd parties and the need for Security and Performance reviews on newly added libraries
  • Regex checks are not that expensive in this case since they are compiled at startup and then are available in the constants lists, it's a pattern that we enforce in the lib when needed

As simply a passionate user who’s been pushing for this to be improved for a bit, assuming this being pushed to code review means good things, just wanted to pop in to say thanks for the work! 👌

@Art.tsymbar or even @MSantos hopefully it’s ok for me to circle back on this but did sign off mean that this issue landed server side?

As the person who first raised this issue to Matt, it only half-fixed it sadly.

-In light-mode it seems you guys tweaked it to appear correctly
-but in dark mode, while it’s no longer completely invisible, it’s rather partially rather than fully obscured

Would have expected the white text on dark background as it appears in light mode to carry over to dark mode as well

Drew Brees and Tom Brady would be two examples (see their team names in the info box that follows their jersey numbers)

I super appreciate the work done so far as I was the one who’s been on this ticket for a while and escalated to @MattCleinman, but perhaps the fix was never case-tested for dark mode?

(Quick aside: Baker Mayfield; scroll down to his stats lower on the wiki page: notice the table headers that are indistinguishable even in light-mode; if that’s fixable too great, but that I question if that’s a contributor maintenance item or a bug, but the fact that Brady and Brees do register properly in light-mode makes me think those are still a symptom of the initial ticket)

@Ijm8710 thanks for the information and indeed we missed the other themes, we will fix it.

Awesome. Thanks @MSantos, glad to hear it does sound like it can be patched as well as opposed to being chalked up to the editors responsibility. Will stay tuned, but probably revisit this once you guys have a chance to go about the 2nd go-around to see if any common edge-cases still slip through that may be able to grabbed in bulk :)

Change 671086 had a related patch set uploaded (by Art.tsymbar; owner: arttsymbar):
[mediawiki/services/mobileapps@master] Disappearing text fix for other than default theme.

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

Change 671086 abandoned by Art.tsymbar:
[mediawiki/services/mobileapps@master] Disappearing text fix for other than default theme.

Reason:

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

Change 671090 had a related patch set uploaded (by Art.tsymbar; owner: arttsymbar):
[mediawiki/services/mobileapps@master] Disappearing text fix for other than default theme.

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

Change 671555 had a related patch set uploaded (by Art.tsymbar; owner: arttsymbar):
[mediawiki/services/mobileapps@master] Custom styling disappearing text fix. Adding color: inherit to the problematic element with 'specific' structure.

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

Change 671090 abandoned by MSantos:
[mediawiki/services/mobileapps@master] Disappearing text fix for other than default theme.

Reason:
in favor of Ifa8ecfd8dd41ffc20b569509497369e344f4e848

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

Hey @MSantos @Art.tsymbar this actually regressed. You guys did patchwork for the light themes and I mentioned the issue persists still in dark mode. I noticed last month on the betas, that the improvement for light mode was reverted as well and it went back to how it was originally. I chose to be patient and wait until the public update though to circle back which was published today, but it’s the same situation. Is that intended?

Hi, @Ijm8710 ! Yeh, the thing is that some further investigation is to be made, as @MSantos mentioned, when we had a chat with him regarding this issue. So there is chance that the change will not be needed. If not, there is a patch for dark(and other) themes cases, which is ready and will be reviewed and merged if needed (after the investigation). Thanks!

Ah ok, that makes total sense, thanks @Art.tsymbar sorry to doubt. Just wanted to make sure it wasn’t an accidental revert. Appreciate the work :)

Hi @MSantos @Art.tsymbar this is looking better (ty for the work thus far) but I still have a few more edge cases that still are extremely similar to the original root post/issue

Unique issue types I haven’t seen reported before

dak Prescott: career stats header is black and unreadable both dark and light mode (this example was fixed, but Matt Davidson (baseball) has exact same scenario)
• Francisco lindor: name header is weird black text on blue background in darker modes. In light mode it knows to use white text correctly
• ^ same lindor page. At bottom of summary chart the word “medals” is almost impossible to see in dark mode but renders properly in light mode (this was patched, then reverted back and the whole rest of the summary table is wonky in dark mode now—Brandon woodruff and others suffer similar fate)
Mary matalin: summary table, political party area it has “before 2016” text. This renders fine in light mode but dark mode that should render white, no?
don mattingly managerial record table, renders fine in light mode but in dark mode notice for the rows in salmon color, the win percentage has black text and looks correct. But games won and lost have white text that doesn’t render as well
• Mars Volta discography page summary table has issues in dark mode
• Non-hyperlinked text in T236712 have issues in that same table still
• nate McMillan head coaching record table’s legend

Issues I’ve encountered, but that already seem to be ticketed in the aggregate theme issue thread (T252904)

• money heist: list of episodes page, the episode titles in the tables are impossible to read in dark mode while rendering fine in light mode (extremely pervasive issue type for many respective individual show pages, plus I see it has now been raised as part of T282295 too)
Mara Volta track listing for Frances the mute was fixed, but rush moving pictures track listing has issues. Seems weird as both appear to have same format. Does this mean issue types like this always have to be raised individually or can they be hit somewhat in aggregate? (Using examples from T282295 so as not to overwhelm, but this is a pretty pervasive issue type as I’ve encountered many on own accord as well)

Would you agree any/all should also be patchable in aggregate like you were able to iron out with the other items I listed?

Thank you

Change 700943 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] [OTRS bug] Disappearing text due to custom styling

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

Hi @Ijm8710 @MSantos !
I've uploaded a patch that solves these recent issues.
I want to emphasize about /Mary_Matalin however. Article was created incorrectly itself (wrong template usage). I fixed this issue in the latest revision.
Issue with black text in /Francisco_Lindor article already fixed in the patch related to T284327
Issue with salmon color rows in /Don_Mattingly fixed in the path related to T282295
We continue working on similar issues. If you find anything else, please, let me know.

Hi @vadim-kovalenko, thanks for the update. Few remarks

•you mentioned T284327, for episode listing for “Ted Lasso”, I would raise the point that in some of the dark themes, the grey background for episode title contrasted with the black background for the episode summary is not what I would expect/a bit jarring. If this was indeed always how the format was, so be it, but I could have sworn this resulted from some of the changes and is due to getting contrast right rather than how that template has always been styled. Wouldn’t it make more sense for episode summary to be lighter grey as well to keep the whole table consistent (and also more in-line with how it’s presented in light mode)? At the very least comparing Ted lassos episode listings with money heist, it’s weird that lasso uses black text on black background for the summary while lasso uses white text on the same black background which reads much better.
•more importantly, unless the patch hasn’t reached the apps yet, I’m seeing a ton of show pages that still have the original issue persist. One example is money heist, but I was hoping that was an across-the-board issue rather than having to be patched for each show individually which does not seem plausible to manage.
• lindor, the original issue has been patched, but the summary table is all out of wack now in dark mode (note I had edited my original comment from a few days ago to note that, as well as noting a few other things new)
• you mentioned T282295, I see some track listing pages are better, but others are not. Also remarked about this above in my edited comment right above (where I gave other track listing examples—I also used markup there to better differentiate still existing vs fixed issues).

3A2FF92C-89C9-4C41-A892-8819A7276D65.png (2×1 px, 3 MB)

69674A9F-E21F-4F75-86D5-B41E56971C16.png (2×1 px, 3 MB)

@MSantos @vadim-kovalenko, while my past two comments went more in depth, still think athlete pages like these are the ones that have been most affected by the recent changes. As mentioned above, the dark pages have actually deprecated since the work started. Should be pretty clear to see the drastic difference between light and dark mode:
•black text on black background
•completely obscured text
•themes missing
•split half-light&half dark panels, etc.

Also, for episode summaries, should there be so many variations for templates.
•episode summaries: sometimes white text but sometimes black on black background
•contrast of summary vs episode titles: sometimes all light theme while others times one has dark background and other has light background

7F034D4F-8D39-4DC9-B898-D5BFA5C2AF3A.png (2×1 px, 474 KB)

9295DBD9-84AD-4347-906E-61AEBCDD53C9.png (2×1 px, 468 KB)

1B59ACCB-85A2-457A-BDAE-F2BF6ADDE67B.png (2×1 px, 400 KB)

*more importantly though, there are still tons of show pages where the episode titles are not contrasted out but completely invisible going back to the root issue (see titles in last image)

2F3015E5-9E1D-438B-8C34-D25575D04246.png (2×1 px, 534 KB)

Any chance to get an update please @MSantos @vadim-kovalenko but any chance to confirm if you agree on any of the more recent comments? Not to be too impatient but I opened this ticket here late in 2020 and have been emailing support even months before that. Understand that merge requests take time but I am hoping to approach this more universally rather than template-by-template if at all possible, but if there’s a 3 month gap between every single time we add a new merge request in this may take years to fix. As shown by some of the images in my comment prior to this, so many articles are affected where not only is text hard to read but even invisible in dark mode. And I’m sure you don’t want to create an experience where we have to constantly bounce between dark and light themes

Hi @Ijm8710 ! We aware of issues you have mentioned. We're working on them in this patch which is currently in progress because at the moment we are looking for a solution that can cover all dark/theme issues which you can take a look at this umbrella task - https://phabricator.wikimedia.org/T252904.

Thanks @vadim-kovalenko for the update. Yes was following that patch but wasn’t sure since it didn’t seem to have an update in a few weeks. Keep in mind I had added some variations even beyond those covered in https://phabricator.wikimedia.org/T252904 above, but I’ll stay tuned

We haven't reached a permanent solution, but we have too many tasks to track the dark/black theme style issues. Because the original bug of this task is resolved, I'll mark it as resolved. Please refer to any further theme style issues at T286643: [EPIC] Systematic solution on preventing rendering issues on user styles between dark/light themes