Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#350 closed defect (fixed)

"Block element" dialog isn’t completely visible when selecting elements inside small frames

Reported by: philll Assignee: kzar
Priority: P3 Milestone: Adblock-Plus-1.8.11-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: smultron45@…, sebastian, trev, manvel@… Blocked By:
Blocking: #1444 Platform: Chrome
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/6264613016436736/

Description (last modified by trev)

Environment

Chrome 34/35
ABP 1.7.4 with no filters active

How to reproduce

  1. Go to doregama.info
  2. Right click on the ad at the extreme right bottom and select ‘Block Element’ from the right click menu

Observed behaviour

The "add Filter(s)" dialog is not completely visible as displayed in attachment:challengebug_5706.png. Works fine if using the "block element" option from the ABP icon menu.

Taken from http://99tests.com/challenges/56/challengebugs/5706

Expected behaviour

The "add Filter(s)" dialog should be completely visible.

Attachments (1)

challengebug_5706.png (69.0 KB) - added by philll 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by philll

  • Description modified (diff)

comment:2 Changed 5 years ago by mapx

  • Cc smultron45@… added

comment:3 Changed 5 years ago by sebastian

The problem here is that the overlay is injected inside an iframe, which is smaller than the overlay. In order to fix this we must always inject the overlay into the top-level frame.

comment:4 Changed 5 years ago by philll

  • Ready set

Changed 5 years ago by philll

comment:5 Changed 5 years ago by philll

  • Description modified (diff)

comment:6 Changed 5 years ago by sebastian

  • Cc sebastian trev added

Alternatively, we could also disable the "Block element" feature in frames that are too small to show the dialog. This would make things way simpler. You could still select the entire frame to block, which might be the preferred behavior in many cases anyway. What do you think, trev and mapx?

comment:7 Changed 5 years ago by mapx

is the first solution you "saw" ("inject the overlay into the top-level frame") too complicated to implement ? top-level frame would be the page itself (where the dialogue window has enough space to be showed), right ?

comment:8 Changed 5 years ago by trev

  • Description modified (diff)

Injecting the dialog into the top frame would certainly be better. I see the following issues:

  • Translating coordinates between frames: window.screenX seems to be supported by both Chrome and Safari, this should give us the position of the frames relative to each other.
  • Communicating with the top frame: we can send a message to all frames and only the frame with parent == window will act upon it.
  • We might not have a content script running in the top frame: we could still fall back to the frame containing the element or just ignore this scenario as it is fairly uncommon.

Other than that, the complication seems to be mostly that additional messaging will be introduced. There should be no significant differences between Chrome and Safari here either. Sebastian, any additional issues that you see here?

comment:9 Changed 5 years ago by sebastian

I don't know of any issues that would prevent us from injecting the dialog always in the top-level frame. However we would have to add a fair amount of complexity, to decouple the code that listens for mouse events, inspects the DOM and finally removes the element and the code rendering and processing the dialog.

I am not sure if that is worth it, considering that this is rather a corner case, in which it seems to be even preferable to block the entire frame. I played a little around with the code and it seems that we could just one check in order to ignore the mouse events in small frames, that if you hover/click inside that frame the parent frame will get the event and you can block the entire frame.

comment:10 Changed 5 years ago by saroyanm

  • Cc manvel@… added
  • Platform set to Unknown

comment:11 Changed 5 years ago by trev

  • Component changed from Unknown to Platform
  • Platform changed from Unknown to Chrome

comment:12 Changed 5 years ago by trev

  • Priority changed from P2 to P3

comment:13 Changed 5 years ago by sebastian

Also see #1443.

comment:14 Changed 5 years ago by greiner

  • Blocking 1444 added

comment:15 Changed 5 years ago by kzar

  • Owner set to kzar

comment:16 Changed 5 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:17 Changed 5 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:18 Changed 5 years ago by sebastian

  • Summary changed from Add Filter(s) dialog is not completely visible for some ads to "Block element" dialog isn’t completely visible when selecting elements inside small frames
Note: See TracTickets for help on using tickets.