Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#1950 closed defect (fixed)

Tooltip not displayed for very long filters

Reported by: mapx Assignee: greiner
Priority: P4 Milestone: Adblock-Plus-2.7.2-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords: goodfirstbug
Cc: trev, arthur, greiner Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29334565/

Description (last modified by trev)

Environment

firefox 36
ABP 2.6.7.3895
easylist, easyprivacy

How to reproduce

-go to (an example)
http://www.caranddriver.com/photo-gallery/2017-ford-focus-rs-official-photos-and-info-news

-open blockable items list
-in the list there is a hidden element ##.advertisement
-hover the mouse above the filter (to see in which list is the filer)
-nothing happens
-the same behaviour is when you search in easylist the filter and then hover above the filter

-the filter is very long, so probably it's some limit ...

1520wbzw.com,760kgu.biz,880thebiz.com,about.com,afro.com,allmusic.com,amctv.com,animax-asia.com,ap.org,araratadvertiser.com.au,areanews.com.au,armidaleexpress.com.au,avclub.com,avonadvocate.com.au,axn-asia.com,barossaherald.com.au,batemansbaypost.com.au,baysidebulletin.com.au,begadistrictnews.com.au,bellingencourier.com.au,bendigoadvertiser.com.au,betvasia.com,bigthink.com,biz1190.com,bizarremag.com,blacktownsun.com.au,blayneychronicle.com.au,bluemountainsgazette.com.au,boingboing.net,bombalatimes.com.au,boorowanewsonline.com.au,bordermail.com.au,braidwoodtimes.com.au,bravotv.com,brimbankweekly.com.au,bunburymail.com.au,business1110ktek.com,business1570.com,businessinsurance.com,busseltonmail.com.au,camdenadvertiser.com.au,camdencourier.com.au,canowindranews.com.au,caranddriver.com,carrierethernetnews.com,caseyweekly.com.au,caseyweeklycranbourne.com.au,centraladvocate.com.au,centralwesterndaily.com.au,cessnockadvertiser.com.au,cinemablend.com,classicandperformancecar.com,clickhole.com,colliemail.com.au,colypointobserver.com.au,competitor.com,coomaexpress.com.au,cootamundraherald.com.au,cowraguardian.com.au,crainsnewyork.com,crookwellgazette.com.au,crosswalk.com,dailyadvertiser.com.au,dailygazette.com,dailyliberal.com.au,dailyrecord.com,dandenongjournal.com.au,defenceweb.co.za,di.fm,donnybrookmail.com.au,downloadcrew.com,dunedintv.co.nz,dungogchronicle.com.au,easternriverinachronicle.com.au,edenmagnet.com.au,elliottmidnews.com.au,esperanceexpress.com.au,essentialmums.co.nz,examiner.com.au,eyretribune.com.au,fairfieldchampion.com.au,fastcocreate.com,fastcodesign.com,financialcontent.com,finnbay.com,forbesadvocate.com.au,frankstonweekly.com.au,gazettextra.com,gematsu.com,gemtvasia.com,gippslandtimes.com.au,gleninnesexaminer.com.au,globest.com,gloucesteradvocate.com.au,goodcast.org,goondiwindiargus.com.au,goulburnpost.com.au,greatlakesadvocate.com.au,grenfellrecord.com.au,guyraargus.com.au,hardenexpress.com.au,hawkesburygazette.com.au,hepburnadvocate.com.au,hillsnews.com.au,hispanicbusiness.com,humeweekly.com.au,huntervalleynews.net.au,i-dressup.com,imgur.com,inverelltimes.com.au,irishtimes.com,juneesoutherncross.com.au,kansas.com,katherinetimes.com.au,kdow.biz,kkol.com,knoxweekly.com.au,lakesmail.com.au,lamag.com,latrobevalleyexpress.com.au,legion.org,lithgowmercury.com.au,liverpoolchampion.com.au,livestrong.com,livetennis.com,macarthuradvertiser.com.au,macedonrangesweekly.com.au,macleayargus.com.au,magtheweekly.com,mailtimes.com.au,maitlandmercury.com.au,mandurahmail.com.au,manningrivertimes.com.au,margaretrivermail.com.au,maribyrnongweekly.com.au,marinmagazine.com,marketwatch.com,maroondahweekly.com.au,meltonweekly.com.au,merimbulanewsonline.com.au,merredinmercury.com.au,metservice.com,monashweekly.com.au,money1055.com,mooneevalleyweekly.com.au,moreechampion.com.au,movies4men.co.uk,mprnews.org,msn.com,mudgeeguardian.com.au,murrayvalleystandard.com.au,muswellbrookchronicle.com.au,myallcoastnota.com.au,nambuccaguardian.com.au,naroomanewsonline.com.au,narrominenewsonline.com.au,nationalgeographic.com,newcastlestar.com.au,northernargus.com.au,northerndailyleader.com.au,northweststar.com.au,nvi.com.au,nynganobserver.com.au,nytimes.com,oann.com,oberonreview.com.au,onetvasia.com,onlinegardenroute.co.za,orange.co.uk,parenthood.com,parkeschampionpost.com.au,parramattasun.com.au,pch.com,peninsulaweekly.com.au,penrithstar.com.au,plasticsnews.com,portlincolntimes.com.au,portnews.com.au,portpirierecorder.com.au,portstephensexaminer.com.au,praguepost.com,psychologytoday.com,queanbeyanage.com.au,racingbase.com,radioguide.fm,redsharknews.com,rhsgnews.com.au,riverinaleader.com.au,roxbydownssun.com.au,rubbernews.com,saitnews.co.za,sconeadvocate.com.au,silverdoctors.com,singletonargus.com.au,smallbusiness.co.uk,sonychannel.co.za,sonychannelasia.com,sonymax.co.za,sonymoviechannel.co.uk,sonytv.com,southcoastregister.com.au,southernhighlandnews.com.au,southernweekly.com.au,southwestadvertiser.com.au,standard.net.au,star-telegram.com,stawelltimes.com.au,stmarysstar.com.au,stonningtonreviewlocal.com.au,summitsun.com.au,suncitynews.com.au,sunjournal.com,sunraysiadaily.com.au,tennantcreektimes.com.au,tenterfieldstar.com.au,theadvocate.com,theadvocate.com.au,thebeachchannel.tv,thecourier.com.au,thecurrent.org,theflindersnews.com.au,theforecaster.net,theguardian.com.au,theherald.com.au,theislanderonline.com.au,theleader.com.au,thenortherntimes.com.au,theridgenews.com.au,therural.com.au,tirebusiness.com,townandcountrymagazine.com.au,transcontinental.com.au,travelpulse.com,twitch.tv,ulladullatimes.com.au,victorharbortimes.com.au,waginargus.com.au,walchanewsonline.com.au,walworthcountytoday.com,washingtonexaminer.com,wauchopegazette.com.au,wellingtontimes.com.au,westcoastsentinel.com.au,westernadvocate.com.au,westernmagazine.com.au,whyallanewsonline.com.au,winghamchronicle.com.au,wollondillyadvertiser.com.au,woot.com,wsj.com,wyndhamweekly.com.au,yasstribune.com.au,yellowpages.ca,youngwitness.com.au##.advertisement

What to change

Blockable items tooltip (hovering any row):

For any filter longer than 482 characters, only show the first and last 240 characters. Replace the middle of the filter by an ellipsis ("\u2026"). Same logic should be applied to the address row as well.

Filter preferences tools (hovering a filter):

Apply the logic outlined above to the filter row.

Change History (15)

comment:1 Changed 4 years ago by mapx

  • Cc arthur added

comment:2 follow-up: Changed 4 years ago by greiner

The issue is that this filter would, with the current implementation, require 63 lines to be added to the tooltip which, much likely, would make the tooltip grow too large anyway. Since it wouldn't make much sense to have a scrollable box inside of a tooltip, I'd suggest truncating long filters (filters with more than 400 characters) and show them in the following form:

1520wbzw.com,760kgu.biz,880thebiz.com,about.com,afro.com,allmusic.com,amctv.com,
animax-asia.com,ap.org,araratadvertiser.com.au,areanews.com.au,armidaleexpress.c
om.au,avclub.com,avonadvocate.com.au,axn-asia.com,barossaherald.com.au,batemansb
aypost.com.au,baysidebulletin.com.au,begadistrictnews.com.au,bellingencourier.co
...
estcoastsentinel.com.au,westernadvocate.com.au,westernmagazine.com.au,whyallanew
sonline.com.au,winghamchronicle.com.au,wollondillyadvertiser.com.au,woot.com,wsj
.com,wyndhamweekly.com.au,yasstribune.com.au,yellowpages.ca,youngwitness.com.au#
#.advertisement

This approach would show the first and last 320 characters of the filter based on the assumption that the middle bit is negligible for display in the tooltip. Would that be sufficient?

comment:3 Changed 4 years ago by greiner

  • Cc greiner added

comment:4 Changed 4 years ago by mapx

it might be a good compromise

comment:5 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by trev

  • Description modified (diff)
  • Keywords goodfirstbug added
  • Priority changed from Unknown to P4
  • Ready set

Unfortunately, I'm unaware of any existing mechanisms that would allow truncating a multiline string. So it does sound like we have to implement our own mechanism.

Replying to greiner:

I'd suggest truncating long filters (filters with more than 400 characters)
...
This approach would show the first and last 320 characters of the filter

So we will truncate filters longer than 400 characters but still happily show 640 characters? This makes no sense to me. Let's rather agree to show the first and last 200 characters of the filter - for anything longer than 402 characters (not for 401 characters because there we would merely replace one character by ellipsis which makes no sense to save space).

comment:6 Changed 4 years ago by trev

  • Description modified (diff)

comment:7 Changed 4 years ago by trev

  • Summary changed from very long filter - viewing problem to Tooltip not displayed for very long filters

comment:8 in reply to: ↑ 5 Changed 4 years ago by greiner

Replying to trev:

Unfortunately, I'm unaware of any existing mechanisms that would allow truncating a multiline string. So it does sound like we have to implement our own mechanism.

We are already creating 80-character long substrings from that string so it shouldn't produce too much overhead as long as we're sticking with multiples of 80.

Replying to greiner:

I'd suggest truncating long filters (filters with more than 400 characters)
...
This approach would show the first and last 320 characters of the filter

So we will truncate filters longer than 400 characters but still happily show 640 characters? This makes no sense to me. Let's rather agree to show the first and last 200 characters of the filter - for anything longer than 402 characters (not for 401 characters because there we would merely replace one character by ellipsis which makes no sense to save space).

I chose 320 since it was a multiple of 80. Admittingly, 400 (5 lines worth of characters) was incorrect… I actually meant 720 (9 lines worth of characters) since this is the maximum lines I imagined we'd want to occupy (as seen in the example). Therefore everything exceeding 720 characters would make sense to be truncated since it would otherwise introduce a tenth line.

comment:9 Changed 4 years ago by trev

  • Description modified (diff)

Right, let's go with multiples of 80 then - I changed the numbers into 240 and 482. I think that seven lines of text should really be enough.

comment:10 Changed 3 years ago by philll

  • Platform changed from Firefox/Firefox Mobile to Firefox

Made Firefox and Firefox mobile available as seperate platforms.

comment:11 Changed 3 years ago by greiner

  • Owner set to greiner
  • Tester set to Unknown
  • Verified working unset

comment:12 Changed 3 years ago by greiner

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

comment:13 Changed 3 years ago by greiner

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 3 years ago by greiner

  • Milestone set to Adblock-Plus-for-Firefox-next

comment:15 Changed 3 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Row width limit and ellipse working in both E10S and non-E10S Firefox.

ABP 2.7.2
Firefox 38.0 / Firefox 46.0a1 (Nightly) / Ubuntu 14.04 x64
Firefox 38.0 / Firefox 43.0.1 / Firefox 46.0a1 (Nightly) / Windows 7 x64

Note: See TracTickets for help on using tickets.