Page MenuHomePhabricator

[OTRS bug] Disappearing text due to custom styling
Open, LowPublic8 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