options.html - Issue 29333819: Issue 2375 - Implement "Blocking lists" section in new options page

Delta Between Two Patch Sets: options.html

Issue 29333819: Issue 2375 - Implement "Blocking lists" section in new options page (Closed)

Left Patch Set: Created Jan. 18, 2016, 9:50 a.m.

Right Patch Set: Fixed the progress indicator and small fixes Created Feb. 4, 2016, 5:43 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 - This file is part of Adblock Plus <https://adblockplus.org/>,	3 - This file is part of Adblock Plus <https://adblockplus.org/>,

4 - Copyright (C) 2006-2015 Eyeo GmbH	4 - Copyright (C) 2006-2016 Eyeo GmbH

5 -	5 -

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

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

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

9 -	9 -

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

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

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

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

14 -	14 -

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

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

17 -->	17 -->

18	18

19 <html>	19 <html>

20 <head>	20 <head>

21 <meta charset="utf-8">	21 <meta charset="utf-8">

22 <title class="i18n_options_page_title"></title>	22 <title class="i18n_options_page_title"></title>

23 <link rel="stylesheet" href="skin/common.css">	23 <link rel="stylesheet" href="skin/common.css">

24 <link rel="stylesheet" href="skin/options.css">	24 <link rel="stylesheet" href="skin/options.css">

25 <style id="search-style"></style>	25 <style id="search-style"></style>

26 <script src="ext/common.js"></script>	26 <script src="ext/common.js"></script>

27 <script src="ext/content.js"></script>	27 <script src="ext/content.js"></script>

28 <script src="common.js"></script>	28 <script src="common.js"></script>

29 <script src="i18n.js"></script>	29 <script src="i18n.js"></script>

30 <script src="options.js"></script>	30 <script src="options.js"></script>

31 </head>	31 </head>

32 <body data-tab="advanced,filter-lists">	32 <body data-tab="general">
Thomas Greiner 2016/01/19 11:27:28 I suppose you added this for testing. Please rever I suppose you added this for testing. Please revert it, if that's the case. saroyanm 2016/01/22 09:55:07 Done. Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > I suppose you added this for testing. Please revert it, if that's the case. Done.
33 <!-- Navigation sidebar -->	33 <!-- Navigation sidebar -->

34 <div id="nav-sidebar">	34 <div id="nav-sidebar">

35 <div id="fixed-sidebar" class="fixed">	35 <div id="fixed-sidebar" class="fixed">

36 <header id="page-title">	36 <header id="page-title">

37 <p class="i18n_options_page_header_1"></p>	37 <p class="i18n_options_page_header_1"></p>

38 <h1 class="i18n_options_page_header_2"></h1>	38 <h1 class="i18n_options_page_header_2"></h1>

39 </header>	39 </header>

40 <nav>	40 <nav>

41 <ul class="tabs vertical">	41 <ul class="tabs vertical">

42 <li id="tab-general" data-action="switch-tab" data-tab="general" cla ss="active">	42 <li id="tab-general" data-action="switch-tab" data-tab="general" cla ss="active">

43 <a class="i18n_options_tab_general"></a>	43 <a class="i18n_options_tab_general"></a>

44 <span class="icon"></span>	44 <span class="icon"></span>

45 </li>	45 </li>

46 <li id="tab-advanced" data-action="switch-tab" data-tab="advanced,fi lter-lists">	46 <li id="tab-advanced" data-action="switch-tab" data-tab="advanced-al lFilterLists">
Thomas Greiner 2016/01/19 11:27:28 Detail: "abc,def" looks like a list of elements bu Detail: "abc,def" looks like a list of elements but it seems that you want to indicate that on page "abc" it should navigate to "def". Therefore I'd suggest to use either "abc/def" or "abc.def" to indicate that hierarchy. Alternatively, you could rewrite it as `data-tab="advanced-filterLists"` to use the `li[data-tab\|="filterLists"]` selector (see https://www.w3.org/TR/css3-selectors/#attribute-selectors). However, since that wouldn't be a hierarchical notation, I'd advise against it. saroyanm 2016/01/22 09:55:07 No, I would like to indicate that the list of tabs Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > Detail: "abc,def" looks like a list of elements but it seems that you want to > indicate that on page "abc" it should navigate to "def". Therefore I'd suggest > to use either "abc/def" or "abc.def" to indicate that hierarchy. No, I would like to indicate that the list of tabs should be active after triggering "switch-tab" action, same way we indicate list of actions, when we would like to trigger several actions. But if you think that we need to have hierarchical model of tab management, we can have it, I just can't see benefit. What you think ? Thomas Greiner 2016/01/25 15:40:27 I see what you mean but in this case they are depe Show quoted text On 2016/01/22 09:55:07, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > Detail: "abc,def" looks like a list of elements but it seems that you want to > > indicate that on page "abc" it should navigate to "def". Therefore I'd suggest > > to use either "abc/def" or "abc.def" to indicate that hierarchy. > No, I would like to indicate that the list of tabs should be active after > triggering "switch-tab" action, same way we indicate list of actions, when we > would like to trigger several actions. > But if you think that we need to have hierarchical model of tab management, we > can have it, I just can't see benefit. What you think ? I see what you mean but in this case they are dependent on each other since switching to tab "filter-lists" doesn't do anything unless you also switch to tab "advanced" so in this case "filter-lists" is dependent on "advanced". Note that this is also relevant for deep-linking which I'll introduce in #2409 that allows us to link to a section of the options page that may be in a different tab. Therefore my suggestion would be to use `data-tab="advanced-filterLists"` so that we can link to it via `options.html#advanced-filterLists` and apply styles via `body[data-tab\|="advanced"]` and `body[data-tab\|="filterLists"]`. saroyanm 2016/01/26 18:36:14 Done. Show quoted text On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:07, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > Detail: "abc,def" looks like a list of elements but it seems that you want > to > > > indicate that on page "abc" it should navigate to "def". Therefore I'd > suggest > > > to use either "abc/def" or "abc.def" to indicate that hierarchy. > > No, I would like to indicate that the list of tabs should be active after > > triggering "switch-tab" action, same way we indicate list of actions, when we > > would like to trigger several actions. > > But if you think that we need to have hierarchical model of tab management, we > > can have it, I just can't see benefit. What you think ? > > I see what you mean but in this case they are dependent on each other since > switching to tab "filter-lists" doesn't do anything unless you also switch to > tab "advanced" so in this case "filter-lists" is dependent on "advanced". > > Note that this is also relevant for deep-linking which I'll introduce in #2409 > that allows us to link to a section of the options page that may be in a > different tab. Therefore my suggestion would be to use > `data-tab="advanced-filterLists"` so that we can link to it via > `options.html#advanced-filterLists` and apply styles via > `body[data-tab\|="advanced"]` and `body[data-tab\|="filterLists"]`. Done.
47 <a class="i18n_options_tab_advanced"></a>	47 <a class="i18n_options_tab_advanced"></a>

48 <span class="icon"></span>	48 <span class="icon"></span>

49 </li>	49 </li>

50 <li id="tab-help" data-action="switch-tab" data-tab="help">	50 <li id="tab-help" data-action="switch-tab" data-tab="help">

51 <a class="i18n_options_tab_help"></a>	51 <a class="i18n_options_tab_help"></a>

52 <span class="icon"></span>	52 <span class="icon"></span>

53 </li>	53 </li>

54 </ul>	54 </ul>

55 </nav>	55 </nav>

56 <a id="link-version">	56 <a id="link-version">

57 <span class="i18n_options_version"></span>	57 <span class="i18n_options_version"></span>

58 <span id="abp-version"></span>	58 <span id="abp-version"></span>

59 </a>	59 </a>

60 <footer>	60 <footer>

61 <ul class="tabs vertical bottom">	61 <ul class="tabs vertical bottom">

62 <li id="tab-share">	62 <li id="tab-share">

63 <a class="i18n_options_tab_share"></a>	63 <a class="i18n_options_tab_share"></a>

64 <span class="icon"></span>	64 <span class="icon"></span>

65 </li>	65 </li>

66 <li id="tab-contribute">	66 <li id="tab-contribute">

67 <a class="i18n_options_tab_contribute"></a>	67 <a class="i18n_options_tab_contribute"></a>

68 <span class="icon"></span>	68 <span class="icon"></span>

69 </li>	69 </li>

70 </ul>	70 </ul>

71 </footer>	71 </footer>

72 </div>	72 </div>

73 </div>	73 </div>

74 <div id="tab-content">	74 <div id="content">

75 <div id="content-wrapper">	75 <div id="content-wrapper">

76 <div id="dialog-background"></div>	76 <div id="dialog-background"></div>

77	77

78 <!-- General tab content -->	78 <!-- General tab content -->

79 <div id="content-general">	79 <div id="content-general" class="tab-content">

80 <div>	80 <div>

81 <h1 class="i18n_options_blocking_title"></h1>	81 <h1 class="i18n_options_blocking_title"></h1>

82 <div class="hbox">	82 <div class="hbox">

83 <div id="blocking-languages">	83 <div id="blocking-languages">

84 <div class="option-name">	84 <div class="option-name">

85 <span class="i18n_options_language_title"></span>	85 <span class="i18n_options_language_title"></span>

86 <div class="tooltip">	86 <div class="tooltip">

87 <span class="i18n_options_readMore"></span>	87 <span class="i18n_options_readMore"></span>

88 <div role="tooltip">	88 <div role="tooltip">

89 <img src="skin/tooltips/block.png" alt="" />	89 <img src="skin/tooltips/block.png" alt="" />

90 <p class="i18n_options_language_title_tooltip"></p>	90 <p class="i18n_options_language_title_tooltip"></p>

91 <div class="notes">	91 <div class="notes">

92 <p class="i18n_options_language_title_tooltip_notes_1">< /p>	92 <p class="i18n_options_language_title_tooltip_notes_1">< /p>

93 <p class="i18n_options_language_title_tooltip_notes_2">< /p>	93 <p class="i18n_options_language_title_tooltip_notes_2">< /p>

94 </div>	94 </div>

95 </div>	95 </div>

96 </div>	96 </div>

97 </div>	97 </div>

98 <ul id="blocking-languages-table" class="table list">	98 <ul id="blocking-languages-table" class="table list">

99 <template>	99 <template>

100 <input type="checkbox" class="control" />	100 <button role="checkbox" class="control"></button>

101 <span class="display"></span>	101 <span class="display"></span>

102 </template>	102 </template>

103 </ul>	103 </ul>

104 <div class="controls">	104 <div class="controls">

105 <button data-action="open-dialog" data-dialog="language">	105 <button data-action="open-dialog" data-dialog="language">

106 <span class="icon icon-add"></span>	106 <span class="icon icon-add"></span>

107 <span class="i18n_options_language_add"></span>	107 <span class="i18n_options_language_add"></span>

108 </button>	108 </button>

109 </div>	109 </div>

110 </div>	110 </div>

111 <div id="further-blocking">	111 <div id="further-blocking">

112 <div class="option-name">	112 <div class="option-name">

113 <strong class="i18n_options_furtherBlocking_title"></strong>	113 <strong class="i18n_options_furtherBlocking_title"></strong>

114 <div class="tooltip">	114 <div class="tooltip">

115 <span class="i18n_options_readMore"></span>	115 <span class="i18n_options_readMore"></span>

116 <div role="tooltip" class="flip-vertical">	116 <div role="tooltip" class="flip-vertical">

117 <img src="skin/tooltips/more.png" alt="" />	117 <img src="skin/tooltips/more.png" alt="" />

118 <p class="i18n_options_furtherBlocking_title_tooltip"></p>	118 <p class="i18n_options_furtherBlocking_title_tooltip"></p>

119 </div>	119 </div>

120 </div>	120 </div>

121 </div>	121 </div>

122 <div id="custom-wrapper">	122 <div id="custom-wrapper">

123 <ul id="recommend-list-table" class="table list">	123 <ul id="recommend-list-table" class="table list">

124 <template>	124 <template>

125 <input type="checkbox" class="control" />	125 <button role="checkbox" class="control"></button>

126 <span class="display"></span>	126 <span class="display"></span>

127 <span class="popular"></span>	127 <span class="i18n_options_popular popular"></span>

128 </template>	128 </template>

129 </ul>	129 </ul>

130 <ul id="custom-list-table" class="table list">	130 <ul id="custom-list-table" class="table list">

131 <template>	131 <template>

132 <input type="checkbox" class="control" />	132 <button role="checkbox" class="control"></button>

133 <span class="display"></span>	133 <span class="display"></span>

134 </template>	134 </template>

135 </ul>	135 </ul>

136 </div>	136 </div>

137 <div class="controls">	137 <div class="controls">

138 <button data-action="open-dialog" data-dialog="custom">	138 <button data-action="open-dialog" data-dialog="custom">

139 <span class="icon icon-add"></span>	139 <span class="icon icon-add"></span>

140 <span class="i18n_options_furtherBlocking_add"><span>	140 <span class="i18n_options_furtherBlocking_add"><span>

141 </button>	141 </button>

142 </div>	142 </div>

(...skipping 11 matching lines...) Expand all Loading...
154 <div role="tooltip">	154 <div role="tooltip">

155 <img src="skin/tooltips/acceptable-ads.png" alt="" />	155 <img src="skin/tooltips/acceptable-ads.png" alt="" />

156 <p class="i18n_options_acceptableAds_title_tooltip_1"></p>	156 <p class="i18n_options_acceptableAds_title_tooltip_1"></p>

157 <p class="i18n_options_acceptableAds_title_tooltip_2"></p>	157 <p class="i18n_options_acceptableAds_title_tooltip_2"></p>

158 <p class="i18n_options_acceptableAds_title_tooltip_3"></p>	158 <p class="i18n_options_acceptableAds_title_tooltip_3"></p>

159 </div>	159 </div>

160 </div>	160 </div>

161 </div>	161 </div>

162 <ul id="acceptableads-table" class="table list">	162 <ul id="acceptableads-table" class="table list">

163 <template>	163 <template>

164 <input type="checkbox" class="control" />	164 <button role="checkbox" class="control"></button>

165 <span class="display"></span>	165 <span class="display"></span>

166 </template>	166 </template>

167 </ul>	167 </ul>

168 </div>	168 </div>

169 <div id="whitelisting">	169 <div id="whitelisting">

170 <div class="option-name">	170 <div class="option-name">

171 <strong class="i18n_options_whitelisted_title"></strong>	171 <strong class="i18n_options_whitelisted_title"></strong>

172 <div class="tooltip">	172 <div class="tooltip">

173 <span class="i18n_options_readMore"></span>	173 <span class="i18n_options_readMore"></span>

174 <div role="tooltip" class="flip-vertical">	174 <div role="tooltip" class="flip-vertical">

(...skipping 29 matching lines...) Expand all Loading...
204 <span></span>	204 <span></span>

205 <button class="i18n_options_button_cancel cancel-button" dat a-action="cancel-domain-exception"></button>	205 <button class="i18n_options_button_cancel cancel-button" dat a-action="cancel-domain-exception"></button>

206 </div>	206 </div>

207 </div>	207 </div>

208 </div>	208 </div>

209 </div>	209 </div>

210 </div>	210 </div>

211 </div>	211 </div>

212	212

213 <!-- Advanced tab content -->	213 <!-- Advanced tab content -->

214 <div id="content-advanced">	214 <div id="content-advanced" class="tab-content">

215 <div>	215 <div>

216 <h1>	216 <h1>

217 <span class="i18n_options_tweaks_title"></span>	217 <span class="i18n_options_tweaks_title"></span>

218 <a class="i18n_options_readMore tooltip" href="#"></a>	218 <a class="i18n_options_readMore tooltip"></a>

219 </h1>	219 </h1>

220 <ul class="table">	220 <ul class="table">

221 <li>	221 <li>

222 <input type="checkbox" id="easylist"/>	222 <button role="checkbox" id="easylist"></button>

223 <span id="block-element-explanation" class="i18n_options_tweaks_ blockElement"></span>	223 <span id="block-element-explanation" class="i18n_options_tweaks_ blockElement"></span>

224 </li>	224 </li>

225 </ul>	225 </ul>

226 </div>	226 </div>

227 <div>	227 <div>

228 <h1>	228 <h1>

229 <span class="i18n_options_blockingList_title"></span>	229 <span class="i18n_options_filterList_title"></span>

230 <a class="i18n_options_readMore tooltip" href="#"></a>	230 <a class="i18n_options_readMore tooltip"></a>

231 </h1>	231 </h1>

232 <ul class="tabs horizontal">	232 <ul class="tabs horizontal">

233 <li class="i18n_options_tab_overview active" data-action="switch-t ab" data-tab="advanced,filter-lists"></li>	233 <li class="i18n_options_tab_overview active" data-action="switch-t ab" data-tab="advanced-allFilterLists"></li>

234 <li class="i18n_options_tab_ownList" data-action="switch-tab" data -tab="advanced,custom-filters"></li>	234 <li class="i18n_options_tab_ownList" data-action="switch-tab" data -tab="advanced-customFilters"></li>

235 </ul>	235 </ul>

236 <div id="blocking-lists">	236 <div id="filter-lists">

237 <div id="filter-lists">	237 <div id="all-filter-lists">

238 <ul class="table cols" id="blocking-lists-table">	238 <div class="table-header">

239 <li class="head static">	239 <h3 class="i18n_options_column_name"></h3>
Thomas Greiner 2016/01/19 11:27:28 The headline should semantically not be part of th The headline should semantically not be part of the list. It's meta data and not a list entry. saroyanm 2016/01/22 09:55:07 Done, it's a bit sad, I couldn't find a way how it Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > The headline should semantically not be part of the list. It's meta data and not > a list entry. Done, it's a bit sad, I couldn't find a way how it would be possible to associate the header with column, hope there will be a solution for that case in future without using table.
240 <span class="i18n_options_tableCol_name"></span>	240 <h3 class="i18n_options_column_date"></h3>

241 <span class="i18n_options_tableCol_date"></span>	241 </div>

242 </li>	242 <ul class="table cols" id="all-filter-lists-table">

243 <template>	243 <template>

244 <input type="checkbox" class="control" />	244 <button role="checkbox" class="control"></button>

245 <span>	245 <div>

246 <span data-action="open-context-menu" class="display"></sp an>	246 <span data-action="open-context-menu" class="display"></sp an>

247 <div class="context-menu">	247 <div data-action="open-context-menu" class="arrow">

248 <a href="#" data-action="open-context-menu" class="arrow "></a>	248 <div class="context-menu">
Thomas Greiner 2016/01/19 11:27:28 I noticed a bunch of positioning styles in "option I noticed a bunch of positioning styles in "options.css". Presumably, that's because the DIV below is not directly linked to this anchor tag. Therefore I'd suggest moving the DIV inside the anchor tag to simplify the styling code. Maybe you could even get rid of the DIV around those two elements. saroyanm 2016/01/22 09:55:07 The problem with double div is that I don't have a Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > I noticed a bunch of positioning styles in "options.css". Presumably, that's > because the DIV below is not directly linked to this anchor tag. Therefore I'd > suggest moving the DIV inside the anchor tag to simplify the styling code. > > Maybe you could even get rid of the DIV around those two elements. The problem with double div is that I don't have a fixed width, so I'm possitioning -50% the inner div inside absolute div, so I can have flexible layout. Also having the anchor outside I can change the visibility of sibling div. Thomas Greiner 2016/01/27 17:16:56 I tried it myself and didn't notice any issues. Wh Show quoted text On 2016/01/22 09:55:07, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > I noticed a bunch of positioning styles in "options.css". Presumably, that's > > because the DIV below is not directly linked to this anchor tag. Therefore I'd > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > Maybe you could even get rid of the DIV around those two elements. > > The problem with double div is that I don't have a fixed width, so I'm > possitioning -50% the inner div inside absolute div, so I can have flexible > layout. > Also having the anchor outside I can change the visibility of sibling div. I tried it myself and didn't notice any issues. What you may have encountered is that the browser was moving your elements outside of the `<a>` element because those are not allowed to have block-elements inside them. The following structure worked for me: <div class="context-menu"> <div class="arrow"> <div class="context-menu-container"> <div class="content"> <a class="update-now"></a> <a class="website"></a> <a class="source"></a> <a class="delete"></a> </div> </div> </div> </div> saroyanm 2016/01/28 17:00:10 Shouldn't the context menu be relative to anchor e Show quoted text On 2016/01/27 17:16:56, Thomas Greiner wrote: > On 2016/01/22 09:55:07, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > I noticed a bunch of positioning styles in "options.css". Presumably, that's > > > because the DIV below is not directly linked to this anchor tag. Therefore > I'd > > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > The problem with double div is that I don't have a fixed width, so I'm > > possitioning -50% the inner div inside absolute div, so I can have flexible > > layout. > > Also having the anchor outside I can change the visibility of sibling div. > > I tried it myself and didn't notice any issues. What you may have encountered is > that the browser was moving your elements outside of the `<a>` element because > those are not allowed to have block-elements inside them. > > The following structure worked for me: > > <div class="context-menu"> > <div class="arrow"> > <div class="context-menu-container"> > <div class="content"> > <a class="update-now"></a> > <a class="website"></a> > <a class="source"></a> > <a class="delete"></a> > </div> > </div> > </div> > </div> Shouldn't the context menu be relative to anchor element ? I think I changed the implementation from some point, can you please comment regarding this in new patch in case you still see some issues. But maybe currently it's implemented in same way I think I forgot to comment about that in some previous reviews. Thomas Greiner 2016/01/29 17:48:07 According to https://issues.adblockplus.org/attach Show quoted text On 2016/01/28 17:00:10, saroyanm wrote: > On 2016/01/27 17:16:56, Thomas Greiner wrote: > > On 2016/01/22 09:55:07, saroyanm wrote: > > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > > I noticed a bunch of positioning styles in "options.css". Presumably, > that's > > > > because the DIV below is not directly linked to this anchor tag. Therefore > > I'd > > > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > > > The problem with double div is that I don't have a fixed width, so I'm > > > possitioning -50% the inner div inside absolute div, so I can have flexible > > > layout. > > > Also having the anchor outside I can change the visibility of sibling div. > > > > I tried it myself and didn't notice any issues. What you may have encountered > is > > that the browser was moving your elements outside of the `<a>` element because > > those are not allowed to have block-elements inside them. > > > > The following structure worked for me: > > > > <div class="context-menu"> > > <div class="arrow"> > > <div class="context-menu-container"> > > <div class="content"> > > <a class="update-now"></a> > > <a class="website"></a> > > <a class="source"></a> > > <a class="delete"></a> > > </div> > > </div> > > </div> > > </div> > > Shouldn't the context menu be relative to anchor element ? According to https://issues.adblockplus.org/attachment/ticket/1526/advanced_3_77.png it should be relative to the arrow. Show quoted text > But maybe currently it's implemented in same way I think I forgot to comment > about that in some previous reviews. I didn't see any code change that would have addressed this. saroyanm 2016/01/29 18:56:16 I think I missread the part where you suggested to Show quoted text On 2016/01/29 17:48:07, Thomas Greiner wrote: > On 2016/01/28 17:00:10, saroyanm wrote: > > On 2016/01/27 17:16:56, Thomas Greiner wrote: > > > On 2016/01/22 09:55:07, saroyanm wrote: > > > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > > > I noticed a bunch of positioning styles in "options.css". Presumably, > > that's > > > > > because the DIV below is not directly linked to this anchor tag. > Therefore > > > I'd > > > > > suggest moving the DIV inside the anchor tag to simplify the styling > code. > > > > > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > > > > > The problem with double div is that I don't have a fixed width, so I'm > > > > possitioning -50% the inner div inside absolute div, so I can have > flexible > > > > layout. > > > > Also having the anchor outside I can change the visibility of sibling div. > > > > > > I tried it myself and didn't notice any issues. What you may have > encountered > > is > > > that the browser was moving your elements outside of the `<a>` element > because > > > those are not allowed to have block-elements inside them. > > > > > > The following structure worked for me: > > > > > > <div class="context-menu"> > > > <div class="arrow"> > > > <div class="context-menu-container"> > > > <div class="content"> > > > <a class="update-now"></a> > > > <a class="website"></a> > > > <a class="source"></a> > > > <a class="delete"></a> > > > </div> > > > </div> > > > </div> > > > </div> > > > > Shouldn't the context menu be relative to anchor element ? > > According to > https://issues.adblockplus.org/attachment/ticket/1526/advanced_3_77.png it > should be relative to the arrow. > > > But maybe currently it's implemented in same way I think I forgot to comment > > about that in some previous reviews. > > I didn't see any code change that would have addressed this. I think I missread the part where you suggested to change anchor tag intially, sorry for that, done.
249 <div role="context-menu">
Thomas Greiner 2016/01/19 11:27:28 This role value doesn't exist. For now, it should This role value doesn't exist. For now, it should be sufficient to use a class. If we want to optimize for accessibility, the structure needs to look like that: <ul role="menu" aria-hidden="true"> <li role="menuitem">...</li> ... </ul> However, there may be other things we need to consider as well (e.g. tab index) so might be best to tackle that in a separate issue. saroyanm 2016/01/22 09:55:06 I see, done. Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > This role value doesn't exist. For now, it should be sufficient to use a class. > > If we want to optimize for accessibility, the structure needs to look like that: > > <ul role="menu" aria-hidden="true"> > <li role="menuitem">...</li> > ... > </ul> > > However, there may be other things we need to consider as well (e.g. tab index) > so might be best to tackle that in a separate issue. I see, done.
250 <div class="content">	249 <div class="content">

251 <a class="update-now" data-action="update-now" href= "#"></a>	250 <a class="i18n_options_filterList_updateNow update-s ubscription" data-action="update-subscription"></a>
Thomas Greiner 2016/01/19 11:27:28 Detail: Please remove hyperlinking to "#" for link Detail: Please remove hyperlinking to "#" for links that don't link anywhere. saroyanm 2016/01/22 09:55:06 Done. Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > Detail: Please remove hyperlinking to "#" for links that don't link anywhere. Done.
252 <a class="website" data-action="website" href="#"></ a>	251 <a class="i18n_options_filterList_website website" t arget="_blank"></a>

253 <a class="source" data-action="source" href="#"></a>	252 <a class="i18n_options_filterList_source source" tar get="_blank"></a>

254 <a class="delete" data-action="delete" href="#"></a>	253 <a class="i18n_options_filterList_delete delete" dat a-action="remove-subscription"></a>

255 </div>	254 </div>

256 </div>	255 </div>

257 </div>	256 </div>

258 </span>	257 </div>

259 <span class="date"></span>	258 <span class="date"></span>

260 <span class="time"></span>	259 <span class="time"></span>

	260 <span class="message"></span>

261 </template>	261 </template>
Thomas Greiner 2016/01/19 11:27:28 This causes the selector `.table.cols li:nth-child This causes the selector `.table.cols li:nth-child(even)` to break the zebra pattern. Instead you could change the selector to `.table.cols li:nth-of-type(even)`. saroyanm 2016/01/22 09:55:07 Done. Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > This causes the selector `.table.cols li:nth-child(even)` to break the zebra > pattern. Instead you could change the selector to `.table.cols > li:nth-of-type(even)`. Done.
262 <li class="static">	262 <li class="static">

263 <input type="checkbox" id="own-list" checked="true" disabled ="true" />	263 <button role="checkbox" class="control" aria-checked="true" disabled="true"></button>
Thomas Greiner 2016/01/19 11:27:28 This checkbox doesn't do anything so either implem This checkbox doesn't do anything so either implement the en-/disabling of this custom subscription or visually indicate that this checkbox is disabled. saroyanm 2016/01/22 09:55:08 Not sure if it looks good, if you think it's ugly Show quoted text On 2016/01/19 11:27:28, Thomas Greiner wrote: > This checkbox doesn't do anything so either implement the en-/disabling of this > custom subscription or visually indicate that this checkbox is disabled. Not sure if it looks good, if you think it's ugly or doesn't fit to current review I'll implement the toggle. Also make sense to decide whether we need to have context menu for this item or not, anyway I'll suggest to revisit user's filter list item management separately. Thomas Greiner 2016/01/25 15:40:27 I don't mind to do it in a separate review but we Show quoted text On 2016/01/22 09:55:08, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > This checkbox doesn't do anything so either implement the en-/disabling of > this > > custom subscription or visually indicate that this checkbox is disabled. > > Not sure if it looks good, if you think it's ugly or doesn't fit to current > review I'll implement the toggle. > Also make sense to decide whether we need to have context menu for this item or > not, anyway I'll suggest to revisit user's filter list item management > separately. I don't mind to do it in a separate review but we should definitely work on that. saroyanm 2016/01/26 18:36:15 Acknowledged. Show quoted text On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:08, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > This checkbox doesn't do anything so either implement the en-/disabling of > > this > > > custom subscription or visually indicate that this checkbox is disabled. > > > > Not sure if it looks good, if you think it's ugly or doesn't fit to current > > review I'll implement the toggle. > > Also make sense to decide whether we need to have context menu for this item > or > > not, anyway I'll suggest to revisit user's filter list item management > > separately. > > I don't mind to do it in a separate review but we should definitely work on > that. Acknowledged.
264 <span>	264 <div>

265 <span class="i18n_options_blockingList_own_list"></span>	265 <span class="i18n_options_filterList_own_list"></span>

266 </span>	266 </div>

267 <span data-action="switch-tab,edit-custom-filters" data-tab= "advanced,custom-filters">	267 <span data-action="switch-tab,edit-custom-filters" data-tab= "advanced-customFilters">

268 <a class="i18n_options_blockingList_edit_own_list" href="# "></a>	268 <a class="i18n_options_filterList_edit_own_list"></a>

269 </span>	269 </span>

270 </li>	270 </li>

271 </ul>	271 </ul>

272 <div class="controls">	272 <div class="controls">

273 <button data-action="open-dialog" data-dialog="custom">	273 <button data-action="open-dialog" data-dialog="custom">

274 <span class="icon icon-add"></span><span class="i18n_options _blockingList_add"></span>	274 <span class="icon icon-add"></span>

	275 <span class="i18n_options_filterList_add"></span>

275 </button>	276 </button>

276 <button data-action="update-all-subscriptions">	277 <button data-action="update-all-subscriptions">

277 <span class="icon icon-update"></span><span class="i18n_opti ons_blockingList_update"></span>	278 <span class="icon icon-update"></span>

	279 <span class="i18n_options_filterList_update"></span>

278 </button>	280 </button>

279 </div>	281 </div>

280 </div>	282 </div>

281 <div id="custom-filters">	283 <div id="custom-filters">

282 <h2 id="custom-filters-header" class="i18n_options_customFilters _title"></h2>	284 <h3 id="custom-filters-header" class="i18n_options_customFilters _title"></h3>

283 <div id="custom-filters-wrapper">	285 <div id="custom-filters-wrapper">

284 <div id="custom-filters-list-wrapper">	286 <div id="custom-filters-list-wrapper">

285 <ul id="custom-filters-table" class="table list">	287 <ul id="custom-filters-table" class="table list">

286 <template>	288 <template>

287 <span class="display"></span>	289 <span class="display"></span>

288 </template>	290 </template>

289 </ul>	291 </ul>

290 <form id="custom-filters-add" class="controls">	292 <form id="custom-filters-add" class="controls">

291 <input type="text"/>	293 <input type="text"/>

292 <button class="i18n_options_button_add" type="submit"></bu tton>	294 <button class="i18n_options_button_add" type="submit"></bu tton>

(...skipping 16 matching lines...) Expand all Loading...
309 <span class="i18n_options_customFilter_save"></span>	311 <span class="i18n_options_customFilter_save"></span>

310 </button>	312 </button>

311 </div>	313 </div>

312 </div>	314 </div>

313 </div>	315 </div>

314 </div>	316 </div>

315 </div>	317 </div>

316 </div>	318 </div>

317	319

318 <!-- Help tab content -->	320 <!-- Help tab content -->

319 <div id="content-help">	321 <div id="content-help" class="tab-content">

320 <h1 class="i18n_options_faq_title"></h1>	322 <h1 class="i18n_options_faq_title"></h1>

321 <p class="i18n_options_faq_description"></p>	323 <p class="i18n_options_faq_description"></p>

322 <p>	324 <p>

323 <a class="i18n_options_faq_title" href="#" target="_blank"></a>	325 <a class="i18n_options_faq_title" href="#" target="_blank"></a>

324 </p>	326 </p>

325 <h1 class="i18n_options_forum_title"></h1>	327 <h1 class="i18n_options_forum_title"></h1>

326 <p class="i18n_options_forum_description"></p>	328 <p class="i18n_options_forum_description"></p>

327 <p>	329 <p>

328 <a class="i18n_options_forum_link" href="#" target="_blank"></a>	330 <a class="i18n_options_forum_link" href="#" target="_blank"></a>

329 </p>	331 </p>

(...skipping 30 matching lines...) Expand all Loading...
360 </ul>	362 </ul>

361 </div>	363 </div>

362 <div id="other-language" class="dialog-content-block">	364 <div id="other-language" class="dialog-content-block">

363 <div>	365 <div>

364 <label for="find-language" class="i18n_options_dialog_language_oth er"></label>	366 <label for="find-language" class="i18n_options_dialog_language_oth er"></label>

365 <input type="search" id="find-language" class="default-focus" />	367 <input type="search" id="find-language" class="default-focus" />

366 </div>	368 </div>

367 <ul id="all-lang-table" class="table list">	369 <ul id="all-lang-table" class="table list">

368 <template>	370 <template>

369 <button class="button-add control">	371 <button class="button-add control">

370 +<span></span>	372 +<span class="i18n_options_button_add"></span>

371 </button>	373 </button>

372 <span class="display"></span>	374 <span class="display"></span>

373 </template>	375 </template>

374 </ul>	376 </ul>

375 </div>	377 </div>

376 </div>	378 </div>

377 <!-- Add custom subscription -->	379 <!-- Add custom subscription -->

378 <div id="dialog-content-custom" class="dialog-content">	380 <div id="dialog-content-custom" class="dialog-content">

379 <div class="dialog-content-block">	381 <div class="dialog-content-block">

380 <div>	382 <div>

381 <label for="blockingList-textbox" class="i18n_options_dialog_custo m_subscription_title"></label>	383 <label for="blockingList-textbox" class="i18n_options_dialog_custo m_subscription_title"></label>

382 <input id="blockingList-textbox" type="text" class="default-focus" placeholder="www.example.com/blockinglist.txt" />	384 <input id="blockingList-textbox" type="text" class="default-focus" placeholder="www.example.com/blockinglist.txt" />

383 </div>	385 </div>

384 <button class="i18n_options_dialog_custom_import" data-action="impor t-subscription"></button>	386 <button class="i18n_options_dialog_custom_import" data-action="impor t-subscription"></button>

385 </div>	387 </div>

386 <div class="dialog-content-block">	388 <div class="dialog-content-block">

387 <h3 class="i18n_options_dialog_edit_own_list"></h3>	389 <h3 class="i18n_options_dialog_edit_own_list"></h3>

388 <button class="i18n_options_dialog_create_own_list" data-action="clo se-dialog,switch-tab,edit-custom-filters" data-tab="advanced,custom-filters"></b utton>	390 <button class="i18n_options_dialog_create_own_list" data-action="clo se-dialog,switch-tab,edit-custom-filters" data-tab="advanced-customFilters"></bu tton>

389 </div>	391 </div>

390 </div>	392 </div>

391 <!-- Add predefined subscription -->	393 <!-- Add predefined subscription -->

392 <div id="dialog-content-predefined" class="dialog-content">	394 <div id="dialog-content-predefined" class="dialog-content">

393 <div class="dialog-content-block">	395 <div class="dialog-content-block">

394 <h3></h3>	396 <h3></h3>

395 <div class="url"></div>	397 <div class="url"></div>

396 <button class="i18n_options_dialog_predefined_confirm default-focus" data-action="add-predefined-subscription"></button>	398 <button class="i18n_options_dialog_predefined_confirm default-focus" data-action="add-predefined-subscription"></button>

397 </div>	399 </div>

398 </div>	400 </div>

399 </div>	401 </div>

400 <!-- Placeholder element to determine when to wrap focus around -->	402 <!-- Placeholder element to determine when to wrap focus around -->

401 <span class="focus-last" tabindex="0"></span>	403 <span class="focus-last" tabindex="0"></span>

402 </div>	404 </div>

403 </body>	405 </body>

404 </html>	406 </html>

LEFT	RIGHT