options.html - Issue 29536764: Issue 5587, 5748 - Use mobile options page on Firefox for Android

Delta Between Two Patch Sets: options.html

Issue 29536764: Issue 5587, 5748 - Use mobile options page on Firefox for Android (Closed) Base URL: https://hg.adblockplus.org/adblockpluschrome/

Left Patch Set: Queue up messages from background page and forward them later Created Sept. 15, 2017, 5:49 p.m.

Right Patch Set: Remove workaround for FOUC issue and update adblockplusui dependency Created Oct. 5, 2017, 1:24 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2	2

3 <!--	3 <!--

4 - This file is part of Adblock Plus <https://adblockplus.org/>,	4 - This file is part of Adblock Plus <https://adblockplus.org/>,

5 - Copyright (C) 2006-present eyeo GmbH	5 - Copyright (C) 2006-present eyeo GmbH

6 -	6 -

7 - Adblock Plus is free software: you can redistribute it and/or modify	7 - Adblock Plus is free software: you can redistribute it and/or modify

8 - it under the terms of the GNU General Public License version 3 as	8 - it under the terms of the GNU General Public License version 3 as

9 - published by the Free Software Foundation.	9 - published by the Free Software Foundation.

10 -	10 -

11 - Adblock Plus is distributed in the hope that it will be useful,	11 - Adblock Plus is distributed in the hope that it will be useful,

12 - but WITHOUT ANY WARRANTY; without even the implied warranty of	12 - but WITHOUT ANY WARRANTY; without even the implied warranty of

13 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	13 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

14 - GNU General Public License for more details.	14 - GNU General Public License for more details.

15 -	15 -

16 - You should have received a copy of the GNU General Public License	16 - You should have received a copy of the GNU General Public License

17 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	17 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

18 -->	18 -->

19	19

20 <meta charset="utf-8">	20 <html>
Thomas Greiner 2017/09/19 17:51:20 Coding style: "Don't omit optional HTML tags." Coding style: "Don't omit optional HTML tags." Manish Jethani 2017/09/19 19:42:05 Done. Show quoted text On 2017/09/19 17:51:20, Thomas Greiner wrote: > Coding style: "Don't omit optional HTML tags." Done.
21 <style>	21 <head>

22 body	22 <meta charset="utf-8">

23 {	23 <style>

24 margin: 0;	24 body

25 border: none;	25 {

26 padding: 0;	26 margin: 0;

27 }	27 border: none;
Thomas Greiner 2017/09/19 17:51:20 Detail: Since you're using fixed positioning to po Detail: Since you're using fixed positioning to position the iFrame, what do you need these rules for? Manish Jethani 2017/09/19 19:42:04 This is a "just in case", for example if the user Show quoted text On 2017/09/19 17:51:20, Thomas Greiner wrote: > Detail: Since you're using fixed positioning to position the iFrame, what do you > need these rules for? This is a "just in case", for example if the user agent style sheet or a user style sheet (inserted by some other extension) sets any of these properties. If a border is set, for example, then it is clearly visible through the iframe since the iframe's background is transparent. While the padding does not affect the position and size of the iframe, it can affect the size of the page, causing a scrollbar to be displayed on the top frame (e.g. "padding: 1000px" to use an extreme example). Maybe we should remove all these and just set "overflow: hidden" since that would be necessary in case there are any UA/user styles that mess with the size of the page. These are not strictly necessary though, I can remove them if you think they clutter the code needlessly. Thomas Greiner 2017/09/20 12:29:59 Optimizing for user styles has never even occurred Show quoted text On 2017/09/19 19:42:04, Manish Jethani wrote: > This is a "just in case", for example if the user agent style sheet or a user > style sheet (inserted by some other extension) sets any of these properties. > > If a border is set, for example, then it is clearly visible through the iframe > since the iframe's background is transparent. > > While the padding does not affect the position and size of the iframe, it can > affect the size of the page, causing a scrollbar to be displayed on the top > frame (e.g. "padding: 1000px" to use an extreme example). > > Maybe we should remove all these and just set "overflow: hidden" since that > would be necessary in case there are any UA/user styles that mess with the size > of the page. > > These are not strictly necessary though, I can remove them if you think they > clutter the code needlessly. Optimizing for user styles has never even occurred to me and it hasn't been discussed yet whether this is a use-case we can/want to optimize for. I don't have a preference towards either though so feel free to leave it like this.
	28 padding: 0;

	29 }

28	30

29 #content	31 #content

30 {	32 {

31 border: none;	33 border: none;

32 position: fixed;	34 position: fixed;

33 top: 0;	35 width: 100%;

34 right: 0;	36 height: 100%;

35 bottom: 0;	37 }

36 left: 0;	38 </style>

37 width: 100%;	39 <script src="options.js" defer></script>

38 height: 100%;	40 </head>

39 }	41 <body>

40 </style>	42 <iframe id="content" data-src="desktop-options.html" data-src-fennec="mobile -options.html"></iframe>

41 <iframe id="content" data-src="desktop-options.html" data-src-fennec="mobile-opt ions.html"></iframe>	43 </body>

42 <script src="options.js" charset="utf-8"></script>	44 </html>
Thomas Greiner 2017/09/19 17:51:20 Suggestion: You could use the "defer" or "async" a Suggestion: You could use the "defer" or "async" attribute if you want to put the script tag inside the document head. Thomas Greiner 2017/09/19 17:51:20 Detail: The "charset" attribute here is redundant Detail: The "charset" attribute here is redundant because it has already been specified for the entire document. Manish Jethani 2017/09/19 19:42:04 Done. Show quoted text On 2017/09/19 17:51:20, Thomas Greiner wrote: > Suggestion: You could use the "defer" or "async" attribute if you want to put > the script tag inside the document head. Done. Manish Jethani 2017/09/19 19:42:04 Done. Show quoted text On 2017/09/19 17:51:20, Thomas Greiner wrote: > Detail: The "charset" attribute here is redundant because it has already been > specified for the entire document. Done.
LEFT	RIGHT