Closed Bug 955081 Opened 10 years ago Closed 10 years ago

[a11y] Tooltips are missing in log viewer

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: aleth)

Details

Attachments

(3 files, 4 obsolete files)

*** Original post on bio 1652 by Tomáš Komárek <tomaskom.cz AT seznam.cz> at 2012-08-16 17:29:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
*** Original post on bio 1652 as attmnt 1811 by tomaskom.cz AT seznam.cz at 2012-08-16 17:29:00 UTC ***

Tooltips are missing in the new log viewer (where message themes apply). 

This is most painful with the Dark theme, where tooltips are the only easy way of displaying when a message was sent. 
(Workaround for getting the time exists: Select and copy the messages to any text editor -> magic copy applies -> you can see the times)

Reproducible: Always - just open log viewer with some content.
*** Original post on bio 1652 at 2012-08-16 17:40:05 UTC ***

Thanks for reporting this!

For some unknown reason, the tooltips for the browser are handled in instantbird.js rather than convbrowser.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
*** Original post on bio 1652 at 2012-08-17 20:22:23 UTC ***

(In reply to comment #1)

> For some unknown reason, the tooltips for the browser are handled in
> instantbird.js rather than convbrowser.

The reason for having this code in instantbird.js is that the tooltip XUL element is in instantbird.xul, so that there's only one for the whole conversation window, rather than one per conversation.
Summary: Tooltips are missing in log viewer → [a11y] Tooltips are missing in log viewer
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1652 as attmnt 2332 at 2013-04-09 17:30:00 UTC ***

Moves the required function from instantbird.js to globalOverlay.js. This means it will also be included (unused) in menus.xml and credits.xhtml. I think we can live with that?
Attachment #8354098 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354098 [details] [diff] [review]
Patch

*** Original change on bio 1652 attmnt 2332 at 2013-04-09 17:41:49 UTC ***

Patching toolkit wasn't intended here.
Attachment #8354098 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1652 as attmnt 2333 at 2013-04-09 18:50:00 UTC ***

Moves the FillHTMLTooltip function to convbrowser instead. Also adds tooltips to the message style preview in Preferences.
Attachment #8354099 - Flags: review?(florian)
Comment on attachment 8354098 [details] [diff] [review]
Patch

*** Original change on bio 1652 attmnt 2332 at 2013-04-09 18:50:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354098 - Attachment is obsolete: true
*** Original post on bio 1652 at 2013-04-09 19:44:36 UTC ***

Why are all these .selectedBrowser needed?
*** Original post on bio 1652 at 2013-04-09 19:45:44 UTC ***

You can also omit the "window." in most (all?) places. It's the global object.
*** Original post on bio 1652 at 2013-04-10 09:44:58 UTC ***

(In reply to comment #6)
> Why are all these .selectedBrowser needed?
In the FillInHTMLTooltip calls, it is only needed in instantbird.xul (because getBrowser returns the tabbrowser, not the convbrowser). But I thought it was more readable if the popupshown handler was the same across tooltips (i.e. if across the code we use the convention that "getBrowser.selectedBrowser" is what we use to get the convbrowser)?
*** Original post on bio 1652 at 2013-04-10 10:35:36 UTC ***

(In reply to comment #8)
> (In reply to comment #6)
> > Why are all these .selectedBrowser needed?
> In the FillInHTMLTooltip calls, it is only needed in instantbird.xul (because
> getBrowser returns the tabbrowser, not the convbrowser).

If it's only needed in instantbird.xul, please only use it in instantbird.xul :-).

> But I thought it was
> more readable if the popupshown handler was the same across tooltips

More code that does nothing, and requires hacks to work (eg the "browser.selectedBrowser = browser;" line in messagestyle.js) is not more readable to me.

> across the code we use the convention that "getBrowser.selectedBrowser" is what
> we use to get the convbrowser)?

The convention is that getBrowser() returns the browser. tabbrowser is a special case, but it attempts to simulate a browser by forwarding the interesting properties (see the "BEGIN FORWARDED BROWSER PROPERTIES.  IF YOU ADD A PROPERTY TO THE BROWSER ELEMENT MAKE SURE TO ADD IT HERE AS WELL." comment).

We haven't forwarded the convbrowser methods in tabbrowser, but if you want to FillInHTMLTooltip to improve code consistency in popupshown handlers that's fine with me.
Comment on attachment 8354099 [details] [diff] [review]
Patch

*** Original change on bio 1652 attmnt 2333 at 2013-04-10 10:35:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354099 - Flags: review?(florian) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1652 as attmnt 2336 at 2013-04-10 11:47:00 UTC ***

Thanks for clearing up my confusion about handling the *browser overload.
Attachment #8354103 - Flags: review?(florian)
Comment on attachment 8354099 [details] [diff] [review]
Patch

*** Original change on bio 1652 attmnt 2333 at 2013-04-10 11:47:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354099 - Attachment is obsolete: true
Comment on attachment 8354103 [details] [diff] [review]
Patch

*** Original change on bio 1652 attmnt 2336 at 2013-04-10 13:35:42 UTC ***

Thanks :-).

If idechix was still creating message themes, he would be very happy to see this getting fixed. I remember the lack of tooltips in the message theme preview was one of his pet bugs :).
Attachment #8354103 - Flags: review?(florian) → review+
*** Original post on bio 1652 at 2013-04-10 22:32:19 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/2a5fcd49ace0

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
*** Original post on bio 1652 as attmnt 2347 at 2013-04-12 12:26:00 UTC ***

This fixes the broken tooltips for text logs, where the tooltip displayed is the previous tooltip (if any) as:
Error: TypeError: getBrowser(...).FillInHTMLTooltip is not a function
Source File: chrome://instantbird/content/viewlog.xul
Line: 1
Attachment #8354114 - Flags: review?(florian)
Attached patch Better followup (obsolete) — Splinter Review
*** Original post on bio 1652 as attmnt 2348 at 2013-04-12 14:24:00 UTC ***

Simpler.
Attachment #8354115 - Flags: review?(florian)
Comment on attachment 8354114 [details] [diff] [review]
Followup to fix tooltips for text logs

*** Original change on bio 1652 attmnt 2347 at 2013-04-12 14:24:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354114 - Attachment is obsolete: true
Attachment #8354114 - Flags: review?(florian)
*** Original post on bio 1652 as attmnt 2349 at 2013-04-12 14:38:00 UTC ***

Add comment (and move handler to the logWindow so there is space for it).
Attachment #8354116 - Flags: review?(florian)
Comment on attachment 8354115 [details] [diff] [review]
Better followup

*** Original change on bio 1652 attmnt 2348 at 2013-04-12 14:38:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354115 - Attachment is obsolete: true
Attachment #8354115 - Flags: review?(florian)
Comment on attachment 8354116 [details] [diff] [review]
Followup to fix tooltips for text logs

*** Original change on bio 1652 attmnt 2349 at 2013-04-12 15:36:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354116 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1652 at 2013-04-13 14:33:09 UTC ***

Follow up in http://hg.instantbird.org/instantbird/rev/3ef77790e0b6
Whiteboard: [checkin-needed]
You need to log in before you can comment on or make changes to this bug.