Closed Bug 1169839 Opened 5 years ago Closed 2 years ago

remove box-sizing: padding-box from Gaia and Gaia tests

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: zentner.kyle, Assigned: zentner.kyle, Mentored)

References

Details

Attachments

(10 files)

46 bytes, text/x-github-pull-request
justindarc
: review-
sfoster
: review+
arthurcc
: review+
alive
: review+
johnhu
: review+
pdahiya
: review-
gerard-majax
: review+
yzen
: feedback+
Details | Review
42 bytes, text/x-github-pull-request
justindarc
: review+
Details | Review
51 bytes, text/x-github-pull-request
pdahiya
: review+
Details | Review
52 bytes, text/x-github-pull-request
Details | Review
37 bytes, text/x-github-pull-request
Details | Review
38 bytes, text/x-github-pull-request
Details | Review
51 bytes, text/x-github-pull-request
pdahiya
: review+
Details | Review
38 bytes, text/x-github-pull-request
Details | Review
41 bytes, text/x-github-pull-request
Details | Review
39 bytes, text/x-github-pull-request
Details | Review
The CSS WG recently resolved to remove box-sizing: padding-box, so Gecko is removing support for it.

However, first we need to stop using it internally.

This bug is for tracking removal of uses from Gaia and associated tests.
Blocks: 1166728
Assignee: nobody → kzentner
Attached file kyle's pull request
https://github.com/mozilla-b2g/gaia/pull/30567

IIRC, the process for getting a Gaia pull request merged is to get review *on bugzilla* (not on github as you might expect).

See: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch

I'm attaching your pull request, to get things going; you should get an appropriate reviewer to take a look.
Comment on attachment 8622622 [details] [review]
kyle's pull request

justidarc, please review apps/camera/style/zoom-bar.css, distros/spark/apps/customizer/js/addon-concat.js, and distros/spark/apps/customizer/js/views/edit.js

pdahiya, please review distros/spark/apps/customizer-launcher/js/element/fxos-dev-mode-dialog.js

sfoster, please review apps/ftu/style/style.css

kaze, please review apps/settings/style/simcard.css

alive, please review apps/system/style/chrome/chrome.css

yzen, please review dev_apps/image-uploader/style/bb/menus-dialogs/core.css

etienne, please review shared/elements/gaia_confirm/style.css, shared/style/confirm.css, and shared/style/value_selector.css

dwi2, please review tv_apps/smart-system/style/chrome/chrome.css

I'm not familiar with who's who and who's available, so this list might need to change.
Attachment #8622622 - Flags: review?(yzenevich)
Attachment #8622622 - Flags: review?(tzhuang)
Attachment #8622622 - Flags: review?(sfoster)
Attachment #8622622 - Flags: review?(pdahiya)
Attachment #8622622 - Flags: review?(jdarcangelo)
Attachment #8622622 - Flags: review?(fabien)
Attachment #8622622 - Flags: review?(etienne)
Attachment #8622622 - Flags: review?(alegnadise+moz)
Comment on attachment 8622622 [details] [review]
kyle's pull request

The Camera change looks ok. However, the Spark apps will need to be patched directly, otherwise the changes you made in this patch will be overwritten the next time the apps get pulled into Gaia. Here are the Spark app repos affected:

Customizer:
https://github.com/fxos/customizer

Customizer Launcher:
https://github.com/fxos/customizer-launcher
Attachment #8622622 - Flags: review?(jdarcangelo) → review-
Attachment #8622702 - Flags: review?(jdarcangelo)
Comment on attachment 8622702 [details] [review]
Customizer pull request.

LGTM. Thanks!
Attachment #8622702 - Flags: review?(jdarcangelo) → review+
This is where the use of padding-box in the customizer-launcher directory ultimately comes from, so I'm fixing it here.
Attachment #8622733 - Flags: review?(pdahiya)
Comment on attachment 8622622 [details] [review]
kyle's pull request

Hi, 

John Hu knows smart-system better than me. And :kaze is inactive for a while, I think Arthur should be able to review it.

Thanks
Attachment #8622622 - Flags: review?(tzhuang)
Attachment #8622622 - Flags: review?(im)
Attachment #8622622 - Flags: review?(fabien)
Attachment #8622622 - Flags: review?(arthur.chen)
Comment on attachment 8622622 [details] [review]
kyle's pull request

LGTM for smart-system part. Thanks.
Attachment #8622622 - Flags: review?(im) → review+
Attachment #8622622 - Flags: review?(alegnadise+moz) → review+
Comment on attachment 8622733 [details] [review]
fxos-dev-mode-dialog pull request.

Hi Kyle

LGTM, See github for updating package.json and bower.json file to version 0.0.4 so that apps using dev dialog component pick this change by using 0.0.4 version. If you can update these two files in this PR , I can help tag and release this update. Thanks!
Attachment #8622733 - Flags: review?(pdahiya) → review+
Comment on attachment 8622622 [details] [review]
kyle's pull request

Hi Kyle
Reviewing this PR for fxos-dev-dialog changes in spark apps. The Spark apps (similar to customizer) will need to be patched in their respective repo to avoid overwriting changes next time the apps get pulled into Gaia. Here are the Spark app repos affected for fxos-dev-dialog:

Customizer Launcher:
https://github.com/fxos/customizer-launcher

Studio
https://github.com/fxos/studio

Sharing
https://github.com/fxos/sharing

Directory
https://github.com/fxos/directory

Once we update fxos-dev-dialog to 0.0.4, the change needed for CL, sharing and directory app is changing version in bower.json file to 0.0.4. e.g. https://github.com/fxos/customizer-launcher/blob/master/bower.json


For studio app, we  need to update manually
https://github.com/fxos/studio/blob/master/bower_components/fxos-dev-mode-dialog/bower.json
https://github.com/fxos/studio/blob/master/bower_components/fxos-dev-mode-dialog/fxos-dev-mode-dialog.js

Thanks!
Attachment #8622622 - Flags: review?(pdahiya) → review-
Comment on attachment 8622622 [details] [review]
kyle's pull request

FTU looks good, thanks.
Attachment #8622622 - Flags: review?(sfoster) → review+
Comment on attachment 8622622 [details] [review]
kyle's pull request

It looks good but I'm not a peer reviewer for these changes, I would as Tim or Vivien. Thanks!
Attachment #8622622 - Flags: review?(yzenevich) → feedback+
Comment on attachment 8622622 [details] [review]
kyle's pull request

Thanks for the feedback everyone!

I'm adding a few tim as a reviewer:

timdream, please review dev_apps/image-uploader/style/bb/menus-dialogs/core.css
Attachment #8622622 - Flags: review?(timdream)
Ouch, I pasted the URL w/o comment: I was meant to say the app was implemented by lissyx so I am deferring the review to him.
Comment on attachment 8622622 [details] [review]
kyle's pull request

r=me for settings app, thanks.
Attachment #8622622 - Flags: review?(arthur.chen) → review+
Attachment #8622622 - Flags: review?(lissyx+mozillians) → review+
Comment on attachment 8622622 [details] [review]
kyle's pull request

Redirecting to a shared/ peer.
Attachment #8622622 - Flags: review?(etienne) → review?(dflanagan)
I'm nominally the owner of shared/ but I really consider myself as owner of shared/js/. I don't know the first thing about shared/style/, not even who wrote most of that stuff, so I'm clearing the review request on me.

Pavel: would you be a good reviewer for the shared/style/ changes in this patch? If not, do you know which of the original Telefonica developers is still active and available to review.

Kyle: the shared CSS code will eventually be replaced by the web components being developed under https://github.com/gaia-components  Separately, you might want to audit those repos and file bugs for any of the components that using padding-box.
Flags: needinfo?(pivanov)
Flags: needinfo?(kzentner)
Attachment #8622622 - Flags: review?(dflanagan)
I've audited all the gaia-components repos and confirmed that there are no uses of box-sizing: padding-box in them.
Flags: needinfo?(kzentner)
I think the two guys who write most of the shared/style are not in the project anymore ... and because I've been involved from the beginning of building blocks I think that I can do this (just need few hours to get my laptop).
Flags: needinfo?(pivanov)
LGTM r+ ... just rebase the PR pls :)
I've updated and rebased the main pull request.

pdahiya, I believe I've made all the appropriate changes, and opened pull requests on all the relevant fxos repos. Thanks for finding them for me.

Did I correctly update the bower.json files?

If so, is there anything else I should change to get r+? Thanks.
Flags: needinfo?(pdahiya)
(In reply to Kyle Zentner from comment #30)
> I've updated and rebased the main pull request.
> 
> pdahiya, I believe I've made all the appropriate changes, and opened pull
> requests on all the relevant fxos repos. Thanks for finding them for me.
> 
> Did I correctly update the bower.json files?
> 
> If so, is there anything else I should change to get r+? Thanks.

Thanks Kyle! Changes in bower.json files look good. I will go ahead and land fxos-dev-mode-dialog PR in #comment 10 and tag and release v0.0.4 of dev-dialog component so that you can request review from respective app owners on opened PRs.
Flags: needinfo?(pdahiya)
Comment on attachment 8627712 [details] [review]
fxos/customizer-launcher pull request

Looks good and has my r+
Attachment #8627712 - Flags: review+
Let me know if you want me to land this in directory (Hackerplace).
Comment on attachment 8627712 [details] [review]
fxos/customizer-launcher pull request

Landed PR with dev-dialog 0.0.4 update for CL on master

https://github.com/fxos/customizer-launcher/commit/79c501b9d2d5e47e1645f494eff8d36e7473065a
(In reply to Michael Henretty [:mhenretty] from comment #34)
> Let me know if you want me to land this in directory (Hackerplace).

Yes, I think it's safe to land directory patch and update dev-dialog to 0.0.4
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.