Page MenuHomePhabricator

Revisit box-sizing: border-box for everything in MobileFrontend
Closed, ResolvedPublic40 Story Points

Description

So yeh the subject of ownership is confusing. Technically MobileFrontend does own the skin, but it doesn't own certain areas which happen to be inside it and I guessbecause of this thus MobileFrontend doesn't own the skin.

I suspect we may want to move the box-sizing rules so that it applies to anything in mobile that is a View (erg I wish there was a url I could point to for our documentation) - this could then be turned off by the VisualEditorOverlay.

I'd still need to look closely at what it does to things like our hacks to get images and tables to render properly.

So next actionable steps I can suggest

  • Let's make the rule only apply in .beta and .stable modes of site to everything and anything that is a View (I'm doing this now) [DONE]
  • Get deployed to cluster.
  • Give me/us time to review in alpha the damage on live wikis if any to the content area.
  • Make a decision based on the above.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 202760 had a related patch set uploaded (by Jdlrobson):
Border box no longer default

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

Change 202760 merged by jenkins-bot:
Border box no longer default

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

Jdlrobson closed this task as Resolved.Apr 16 2015, 5:27 PM

So while the latest patch fixes VE, I'd still like to switch all views over to not use the border-box * selector. Any components of MF that may get shared between read mode & VE (for example, the toolbar) could render differently. Also future components that might be embedded in the regular view, but which aren't owned by MF may have the same problem as VE.

Esanders reopened this task as Open.Jun 13 2015, 11:08 AM

This appears to not be working correctly. The view-border-box class is getting applied to the body before VE loads, and never removed.

Change 218608 had a related patch set uploaded (by Esanders):
Never apply border-box to 'body' (i.e. for a 'Skin')

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

Change 218608 merged by jenkins-bot:
Never apply border-box to 'body' (i.e. for a 'Skin')

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

Note: The latest fix has introduced some visual regressions which have made it out on to 1.26wmf10. See T102868: Big logout icon we need to decide whether to revert this or work on top of it some fixes (please don't rush changes to the box-sizing defaults in future without vigorous testing across the whole of the mobile site)

Once the patch is reverted, I'll restore it and we'll review/test it a bit more vigorously second time round to fix up the remaining issues with content inside the skin.

Change 223176 had a related patch set uploaded (by Jdlrobson):
Skin itself should not be border box

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

GOIII added a subscriber: GOIII.Jul 7 2015, 12:57 AM

I'm a bit lost here -- and totally admit that could just be my lack of understanding re: how LESS "works" or something so I apologize in advance if this is rather 'obvious' to you coder types -- but what exactly is changed by r223176 of July 6th if the .view-border-box class is still present in the BODY element and still defined to cascade down to any & all of it's decendants &/or children elements (.view-border-box *, .view-border-box)?

Sure - I can grasp the fact the class name was added to certain elements - I'm just not sure what difference that makes if the BODY element's cascade is still in place (or is that stopped by the added bits to Skin.js?).

I have to ask because when halting that "body cascade" was the aim two or three weeks ago, it had to be reverted in r219877 to recover from all the "deviations" applying it had exposed in process.

Thanks for your time at any rate.

@GOIII: 219877 was reverted because it caused a handful of regressions.

As of 223176, the view-border-box class won't be present on the body element, which is the result of the change to the Skin class as you pointed out. So the major difference between the initial and current patches is that the latter also addresses the regressions caused by the change.

phuedx added a subscriber: Florian.Jul 7 2015, 3:10 PM

The file that I was trying to share on 223176 was

/cc @Florian

GOIII added a comment.Jul 8 2015, 3:27 AM

Thanks for the clarification, phuedx.

Still think this is a bad idea; fixed skin design and format should use one setting (imho: border-box) and the branch/container dealing with user initiated content, templates and the like should also have one setting (in this case; content-box).

Eliminating the attribute & allowing the default (content-box) to reign simply by attribute omission doesn't seem to be the optimal way forward.

Change 223176 merged by jenkins-bot:
Skin itself should not be border box

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

This is merged. @Esanders any follow ups required. Does VE look good to you? (All looks well to me)

Jhernandez closed this task as Resolved.Jul 29 2015, 10:43 AM
Jhernandez added a subscriber: Jhernandez.

Thanks @Jdlrobson for all the work on this.

Jdforrester-WMF moved this task from To Triage to Epics on the VisualEditor board.Jul 30 2015, 3:52 PM

@Jdforrester-WMF set Story Points to 40.

Wat?

sounds pretty accurate, but hindsight is 20/20 ;)

kaldari removed a subscriber: kaldari.Aug 14 2015, 6:11 PM
Esanders reopened this task as Open.Aug 15 2015, 11:46 AM

It's certainly fixed for VE, which was the high-priority task.

However there is still a * selector left over, which is going to cause problems in the future if any other code is integrated with MF. This will need to be resolved before we migrate to OOUI.

Jdlrobson closed this task as Resolved.Jan 9 2019, 6:51 PM

Title of description is a little confusing as we're not planning on revisiting it in MobileFrontend. I think this is superseded by T147692.
If this needs to be reopened, please flesh out the task description.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJan 9 2019, 6:51 PM