Issue 29363476: Issue 2879 - Move element selection into the content process (Closed)
|
Created:
Nov. 17, 2016, 1:17 p.m. by Wladimir Palant Modified:
Dec. 1, 2016, 9:40 a.m. Reviewers:
saroyanm Base URL:
https://hg.adblockplus.org/elemhidehelper Visibility:
Public. |
DescriptionThis is mostly done, only the view source commands haven`t been updated and won`t work.
Patch Set 1 #
Total comments: 64
Patch Set 2 : Addressed comments and marked extension as E10S-compatible #
MessagesTotal messages: 5
Unfortunately, even if I copied aardvark.js to child/selection.js the resulting diff wouldn't be of much help. The changes were mostly straightforward, I'll try to indicate the places where they are not. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js File lib/aardvark.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:93: "_blank", "chrome,centerscreen,resizable,dialog=no", nodeInfo); This function is pretty much what Aardvark.select() used to be. With this function now being called from the child, it no longer needs to request the node info so things got a lot simpler. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:116: E = id => null; This function is what Aardvark.quit() used to be. With the actual command being implemented in the child, this only does the necessary cleanup in the parent now. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:123: showFeedback = this[command](); The command handler no longer gets the selected element, this one is only available in the child. The two commands remaining in the parent will need to message the child in order to get the necessary information, this hasn't been implemented yet. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:126: showFeedback = (command != "select" && command != "quit"); This changes behavior slightly - e.g. you will get visual feedback for the "wider" command even if you are at the top level already. We could implement the old behavior where you only get visual feedback if something actually changes (the commands would have to message the parent in order to show the visual feedback then) but I doubt that it is worth the added complexity. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:276: if (sourceBox.state == "open") This is a behavior change, originally we would only hide the source code tooltip if the selection was unchanged, otherwise we would show the source of the new selection. The complexity here doesn't seem to be worth it so now this command will always hide an already visible tooltip. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.js File lib/child/commands.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:101: let blinkState = null; I moved the properties related to the blink functionality into their own object in order to achieve code locality - everything related to blinking is located right here. One consequence is: canceling element selection will no longer cancel blinking. Given that blinking stops after 1.5 seconds anyway this doesn't seem to be an issue. However, I had to add checks for the case where the page unloads while we are still blinking (look for Cu.isDeadWrapper). This might not have been an issue before because pagehide event cancels selection meaning that blinking would be canceled as well. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:111: function doBlink() This was a closure in the original code, having it like that with the entire state stored globally seemed pointless however. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:124: ); Original code was simply assigning element.style.visibility, I added !important here. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection.js File lib/child/selection.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:16: let state = exports.state = {}; Original code was storing the selection state as properties of the Aardvark object. I thought that a separate object for that would be better, already because resetting state will be much easier then. Unfortunately, resetting is a bit more complicated than I hoped for - this object is exported, so we cannot just set it to null or replace with a different object. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:104: ); This changes the original logic which was simply hardcoding a bunch of schemes. Adblock Plus exposes the schemes where element hiding won't apply so we can just use that preference. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:216: let delta = event.deltaY || event.deltaX; This was dead code originally, using DOMMouseScroll event that became unsupported. I didn't want to commit an untested dead codepath so I changed it to use the wheel event. I found that scrolling vertically via trackpad would set event.deltaY. Scrolling with a hardware mouse and a mouse wheel however would set event.deltaX. We require Shift to be pressed here and at least on OS X it changes the scrolling direction for the mouse wheel. That's why I had to accept both vertical and horizontal scrolling here, unlike the original code. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:272: function onAfterPaint(event) This code path is untested, I'm not sure whether it works at all. Its original purpose was keeping up with scrolling but it definitely doesn't do that any more - not in the original code either. This will need to be fixed later, we need to listen to other events. https://codereview.adblockplus.org/29363476/diff/29363477/lib/windowWrapper.js File lib/windowWrapper.js (left): https://codereview.adblockplus.org/29363476/diff/29363477/lib/windowWrapper.j... lib/windowWrapper.js:91: item.setAttribute("disabled", "true"); The menu item used to indicate if Element Hiding Helper wouldn't run on some page. Now that canSelect moved to the child process this got somewhat more complicated and removed that indicator for now. I think that I will need to bring it back - show the menu as enabled initially, and set "disabled" attribute after receiving a response from child.
> only the view source commands haven`t been updated and won`t work I assume I should expect another review. Sorry that it took a while. I addressed my questions in the review instead of writing in IRC. The most important question I think it's about the separation in selection.js about the process separation. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js File lib/aardvark.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:2: * This Source Code is subject to the terms of the Mozilla Public License Irrelevant: Why is this file called Aadvark ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:18: messageManager.addMessageListener("ElemHideHelper:SelectionStarted", selectionStarted); Nit: Exceeding 80 chars https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:53: rememberedWrapper: null, Curious: Why rememberedWrapper, but not just wrapper ? Do we clear it's state because of memory leak ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:63: if ("selectedBrowser" in browser) What is "selectedBrowser" property ? Are we assigning it ? Or is it just not documented here -> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:77: this.browser = wrapper.browser; We already assigning this.rememberedWrapper value to "this.browser" in "start" method. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:92: this.window.openDialog("chrome://elemhidehelper/content/composer.xul", Shouldn't we also remove eventlisteners on selection succeeded or call selectionstopped ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:93: "_blank", "chrome,centerscreen,resizable,dialog=no", nodeInfo); On 2016/11/17 13:54:47, Wladimir Palant wrote: > This function is pretty much what Aardvark.select() used to be. With this > function now being called from the child, it no longer needs to request the node > info so things got a lot simpler. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:116: E = id => null; On 2016/11/17 13:54:47, Wladimir Palant wrote: > This function is what Aardvark.quit() used to be. With the actual command being > implemented in the child, this only does the necessary cleanup in the parent > now. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:123: showFeedback = this[command](); On 2016/11/17 13:54:47, Wladimir Palant wrote: > The command handler no longer gets the selected element, this one is only > available in the child. The two commands remaining in the parent will need to > message the child in order to get the necessary information, this hasn't been > implemented yet. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:126: showFeedback = (command != "select" && command != "quit"); On 2016/11/17 13:54:47, Wladimir Palant wrote: > This changes behavior slightly - e.g. you will get visual feedback for the > "wider" command even if you are at the top level already. We could implement the > old behavior where you only get visual feedback if something actually changes > (the commands would have to message the parent in order to show the visual > feedback then) but I doubt that it is worth the added complexity. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:217: if (event.keyCode == event.DOM_VK_ESCAPE) Note: keyCode is a deprecated property. You want me to file a separate issue for that ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.js File lib/child/commands.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:45: stopSelection(); Why not call stopSelection in SelectionSucceeded method, IMO that should be handled by that method, so we do not need call it each time we are calling SelectionSucceeded event. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:101: let blinkState = null; On 2016/11/17 13:54:48, Wladimir Palant wrote: > I moved the properties related to the blink functionality into their own object > in order to achieve code locality - everything related to blinking is located > right here. One consequence is: canceling element selection will no longer > cancel blinking. Given that blinking stops after 1.5 seconds anyway this doesn't > seem to be an issue. However, I had to add checks for the case where the page > unloads while we are still blinking (look for Cu.isDeadWrapper). This might not > have been an issue before because pagehide event cancels selection meaning that > blinking would be canceled as well. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:111: function doBlink() On 2016/11/17 13:54:48, Wladimir Palant wrote: > This was a closure in the original code, having it like that with the entire > state stored globally seemed pointless however. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:124: ); On 2016/11/17 13:54:48, Wladimir Palant wrote: > Original code was simply assigning element.style.visibility, I added !important > here. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection.js File lib/child/selection.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:1: /* I thought that this code belongs to Aardvark ? What is the reason of separation ? My assumption: It's related to e10s. How we separate processes ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:16: let state = exports.state = {}; On 2016/11/17 13:54:48, Wladimir Palant wrote: > Original code was storing the selection state as properties of the Aardvark > object. I thought that a separate object for that would be better, already > because resetting state will be much easier then. Unfortunately, resetting is a > bit more complicated than I hoped for - this object is exported, so we cannot > just set it to null or replace with a different object. We are setting the properties of state in other function and it's not obvious what properties it should hold, why not to assign null, so it would be obvious what properties state suppose to have ? Or at least document them. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:40: wnd.addEventListener("wheel", onMouseScroll, true); It's a deprecated event: https://developer.mozilla.org/en-US/docs/Web/API/MouseWheelEvent According to the description of "mousemove" event this should be covered by MouseMove event: https://developer.mozilla.org/en/docs/Web/Events/mousemove https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:86: let acceptlocalfiles; Nit: camelCase is missing https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:89: acceptlocalfiles = Services.prefs.getBoolPref("extensions.elemhidehelper.acceptlocalfiles"); Nit: exceeding 80 chars. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:120: let tagName = elem.localName; localName is a deprecated property: https://developer.mozilla.org/en-US/docs/Web/API/Node/localName https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:127: addition += ", style: " + elem.style.cssText; Weak suggestion: What about mentioning that it's na inline-style not style in general, because we are not showing the styles applied by ID selectors and Classnames, may lead to confusion. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:132: function setAnchorElement(anchor) What is the anchor element ? I assume it's not related to "a tag"? https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:169: state.boxElement.style.left = Math.min(pos.left - 1, wndWidth - 2) + "px"; Is this approach even working with RTL websites ? seems like we are only considering the LTR direction. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:216: let delta = event.deltaY || event.deltaX; On 2016/11/17 13:54:48, Wladimir Palant wrote: > This was dead code originally, using DOMMouseScroll event that became > unsupported. I didn't want to commit an untested dead codepath so I changed it > to use the wheel event. > > I found that scrolling vertically via trackpad would set event.deltaY. Scrolling > with a hardware mouse and a mouse wheel however would set event.deltaX. We > require Shift to be pressed here and at least on OS X it changes the scrolling > direction for the mouse wheel. That's why I had to accept both vertical and > horizontal scrolling here, unlike the original code. Acknowledged. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:220: let commands = require("./commands"); Detail: we already loading the module in onMouseClick event, why not to load it in the beginning of the file ? https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:272: function onAfterPaint(event) On 2016/11/17 13:54:48, Wladimir Palant wrote: > This code path is untested, I'm not sure whether it works at all. Its original > purpose was keeping up with scrolling but it definitely doesn't do that any more > - not in the original code either. This will need to be fixed later, we need to > listen to other events. We do have "onMouseScroll" function, I thought it should take care of your mentioned issue. I'm not really sure if we need this event, sounds like it's being fired whenever something is being changed/drawn on the screen, feels like we are covering all edge cases, but yes maybe we can discuss that in separate issue. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/utils.js File lib/child/utils.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/utils.js#... lib/child/utils.js:74: function intersectRect(rect, wnd) Suggestion: This function will be created each time we will run "getElementPosition", let's move it outside of the function. https://codereview.adblockplus.org/29363476/diff/29363477/lib/windowWrapper.js File lib/windowWrapper.js (left): https://codereview.adblockplus.org/29363476/diff/29363477/lib/windowWrapper.j... lib/windowWrapper.js:91: item.setAttribute("disabled", "true"); On 2016/11/17 13:54:48, Wladimir Palant wrote: > The menu item used to indicate if Element Hiding Helper wouldn't run on some > page. Now that canSelect moved to the child process this got somewhat more > complicated and removed that indicator for now. I think that I will need to > bring it back - show the menu as enabled initially, and set "disabled" attribute > after receiving a response from child. Acknowledged.
https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js File lib/aardvark.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:2: * This Source Code is subject to the terms of the Mozilla Public License On 2016/11/23 17:44:37, saroyanm wrote: > Irrelevant: Why is this file called Aadvark ? The origin of this code is the Aardvark extension for Firefox (now retired in favor of the bookmarklet): http://www.karmatics.com/aardvark/. I convinced the author to provide it under the MPL so that I could build EHH around it. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:18: messageManager.addMessageListener("ElemHideHelper:SelectionStarted", selectionStarted); On 2016/11/23 17:44:37, saroyanm wrote: > Nit: Exceeding 80 chars Done. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:53: rememberedWrapper: null, On 2016/11/23 17:44:37, saroyanm wrote: > Curious: Why rememberedWrapper, but not just wrapper ? > Do we clear it's state because of memory leak ? Its a temporary state, a bit of a hack so that we don't need to figure out which wrapper the message refers to. But I suspect that we'll need to do this properly later on... https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:63: if ("selectedBrowser" in browser) On 2016/11/23 17:44:38, saroyanm wrote: > What is "selectedBrowser" property ? > Are we assigning it ? > Or is it just not documented here -> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser You need to look at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabbrowser. In Firefox and SeaMonkey we'll get a <xul:tabbrowser>, in Thunderbird a <xul:browser>. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:77: this.browser = wrapper.browser; On 2016/11/23 17:44:38, saroyanm wrote: > We already assigning this.rememberedWrapper value to "this.browser" in "start" > method. No, we are only assigning it to the local browser variable. start method doesn't save any state, this is intentional - selection might not start, e.g. if you are on a special page. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:92: this.window.openDialog("chrome://elemhidehelper/content/composer.xul", On 2016/11/23 17:44:37, saroyanm wrote: > Shouldn't we also remove eventlisteners on selection succeeded or call > selectionstopped ? No, the select command (in lib/child/commands.js) will call stopSelection() after sending SelectionSucceeded message. So we will get a SelectionStopped message as well. https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:217: if (event.keyCode == event.DOM_VK_ESCAPE) On 2016/11/23 17:44:38, saroyanm wrote: > Note: keyCode is a deprecated property. > You want me to file a separate issue for that ? Yes, definitely. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.js File lib/child/commands.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/commands.... lib/child/commands.js:45: stopSelection(); On 2016/11/23 17:44:38, saroyanm wrote: > Why not call stopSelection in SelectionSucceeded method, IMO that should be > handled by that method, so we do not need call it each time we are calling > SelectionSucceeded event. stopSelection() is a general cleanup method. It doesn't merely send the SelectionStopped message so that the parent knows it needs to cleanup, it also performs some cleanup in the child - like removing event handlers. It actually makes more sense to have only one cleanup path. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection.js File lib/child/selection.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:1: /* On 2016/11/23 17:44:39, saroyanm wrote: > I thought that this code belongs to Aardvark ? > What is the reason of separation ? > My assumption: It's related to e10s. How we separate processes ? Yes, E10S is the reason for this whole change. Any code accessing webpages needs to go into the child process. The parent on the other hand can mess with the browser UI (e.g. our tooltips). The two subsystems can communicate via messaging. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:16: let state = exports.state = {}; On 2016/11/23 17:44:39, saroyanm wrote: > We are setting the properties of state in other function and it's not obvious > what properties it should hold, why not to assign null, so it would be obvious > what properties state suppose to have ? Or at least document them. Setting them here doesn't make sense - the definition of an empty state is that no properties are there (which also makes it easy to clean it up). But I guess that we can document them now, the naming is pretty non-obvious. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:40: wnd.addEventListener("wheel", onMouseScroll, true); On 2016/11/23 17:44:39, saroyanm wrote: > It's a deprecated event: > https://developer.mozilla.org/en-US/docs/Web/API/MouseWheelEvent That page is about the mousewheel event, not wheel. You should be looking here: https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:86: let acceptlocalfiles; On 2016/11/23 17:44:39, saroyanm wrote: > Nit: camelCase is missing Done. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:89: acceptlocalfiles = Services.prefs.getBoolPref("extensions.elemhidehelper.acceptlocalfiles"); On 2016/11/23 17:44:39, saroyanm wrote: > Nit: exceeding 80 chars. Done. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:120: let tagName = elem.localName; On 2016/11/23 17:44:39, saroyanm wrote: > localName is a deprecated property: > https://developer.mozilla.org/en-US/docs/Web/API/Node/localName That would have been surprising - the docs say that it was removed in Firefox 48 yet I tested this code path successfully. In fact, it says below that only Node.localName is deprecated. Element.localName on the other hand is alive and kicking: https://developer.mozilla.org/en-US/docs/Web/API/Element/localName https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:127: addition += ", style: " + elem.style.cssText; On 2016/11/23 17:44:39, saroyanm wrote: > Weak suggestion: What about mentioning that it's na inline-style not style in > general, because we are not showing the styles applied by ID selectors and > Classnames, may lead to confusion. Hasn't lead to any confusion so far, we can just as well leave it then :) https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:132: function setAnchorElement(anchor) On 2016/11/23 17:44:39, saroyanm wrote: > What is the anchor element ? I assume it's not related to "a tag"? Nope. I've documented the state properties now, should hopefully make it more obvious. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:169: state.boxElement.style.left = Math.min(pos.left - 1, wndWidth - 2) + "px"; On 2016/11/23 17:44:38, saroyanm wrote: > Is this approach even working with RTL websites ? > seems like we are only considering the LTR direction. Well, it works with RTL websites. Our UI is always displayed in LTR direction however, even if extension locale (which is what's relevant here, not the webpage) is Hebrew. We probably want to file an issue on that, not trivial to fix however. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:220: let commands = require("./commands"); On 2016/11/23 17:44:38, saroyanm wrote: > Detail: we already loading the module in onMouseClick event, why not to load it > in the beginning of the file ? That would create a circular reference. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:272: function onAfterPaint(event) On 2016/11/23 17:44:39, saroyanm wrote: > We do have "onMouseScroll" function, I thought it should take care of your > mentioned issue. Given that the purpose of this function is selecting via mouse wheel - nope. It's merely a confusing name that we should change occasionally. > I'm not really sure if we need this event, sounds like it's being fired whenever > something is being changed/drawn on the screen, feels like we are covering all > edge cases, but yes maybe we can discuss that in separate issue. The idea was covering things like DOM modifications and scrolling here. I'm fairly certain that it was working, that was a long time ago however. So - yes, we should definitely look into this but it's not a very straightforward change. https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/utils.js File lib/child/utils.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/utils.js#... lib/child/utils.js:74: function intersectRect(rect, wnd) On 2016/11/23 17:44:39, saroyanm wrote: > Suggestion: This function will be created each time we will run > "getElementPosition", let's move it outside of the function. Done.
Thanks for detailed answers, I've acknowledged all your comments. LGTM https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js File lib/aardvark.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/aardvark.js#new... lib/aardvark.js:217: if (event.keyCode == event.DOM_VK_ESCAPE) On 2016/11/24 14:02:01, Wladimir Palant wrote: > On 2016/11/23 17:44:38, saroyanm wrote: > > Note: keyCode is a deprecated property. > > You want me to file a separate issue for that ? > > Yes, definitely. Done -> #4666 https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection.js File lib/child/selection.js (right): https://codereview.adblockplus.org/29363476/diff/29363477/lib/child/selection... lib/child/selection.js:169: state.boxElement.style.left = Math.min(pos.left - 1, wndWidth - 2) + "px"; On 2016/11/24 14:02:01, Wladimir Palant wrote: > On 2016/11/23 17:44:38, saroyanm wrote: > > Is this approach even working with RTL websites ? > > seems like we are only considering the LTR direction. > > Well, it works with RTL websites. Our UI is always displayed in LTR direction > however, even if extension locale (which is what's relevant here, not the > webpage) is Hebrew. We probably want to file an issue on that, not trivial to > fix however. Here you go: #4665 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
