Closed Bug 1537729 Opened 6 years ago Closed 5 years ago

remove grid usage from comm/mail/base/content/msgHdrView.inc.xul

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(6 files, 5 obsolete files)

Screenshot 2019-09-10 at 6.00.16 PM.png
(deleted), image/png
Details
msgHdrinc_macOS.png
(deleted), image/png
Details
msgHdrInc_Ubuntu.png
(deleted), image/png
Details
Screenshot 2019-09-18 at 10.42.31 PM.png
(deleted), image/png
Details
Screenshot 2019-09-18 at 10.30.36 PM.png
(deleted), image/png
Details
Bug-1537729_remove_grid_msgHdrView-inc-xul.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

I find this grid removal important and complex so I have made a separate bug for this.

Assignee: nobody → khushil324
Blocks: tb-burn-xul-grids
Status: NEW → ASSIGNED
Group: mail-core-security
Status: ASSIGNED → NEW
Type: enhancement → task
Attachment #9087096 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9087096 [details] [diff] [review] Bug-1537729_remove-grid_msgHdrView-inc-xul.patch Review of attachment 9087096 [details] [diff] [review]: ----------------------------------------------------------------- There seems to be more padding/margins than there used to be (around each row). The header name (From, Subject etc) is also not styled the same as it used to (now darker). I think the header value is also missing it's style. Something's probably off for the width calculations too. It doesn't "fill up" the lines properly, and wraps at another (worse) place than it's supposed to. ::: mail/base/content/msgHdrView.inc.xul @@ +222,5 @@ > .clientWidth of its largest contained child by an unspecified > amount. --> > + <html:table id="expandedHeaders"> > + <html:tr id="expandedfromRow"> > + <html:th id="expandedfromTH"> would be nicer to name it expandedfromTableHeader (similarly with the other ths) ::: mail/base/content/msgHdrView.js @@ +753,5 @@ > // we may need to collapse or show the tag header row... > + if (!headerEntry.valid) { > + headerEntry.enclosingRow.setAttribute("hidden", "hidden"); > + } else { > + headerEntry.enclosingRow.removeAttribute("hidden"); you could just write eaderEntry.enclosingRow.toggleAttribute("hidden", !headerEntry.valid);
Attachment #9087096 - Flags: review?(mkmelin+mozilla) → review-

(In reply to Magnus Melin [:mkmelin] from comment #2)

Something's probably off for the width calculations too. It doesn't "fill
up" the lines properly, and wraps at another (worse) place than it's
supposed to.

Can you explain this a little?

Attachment #9087096 - Attachment is obsolete: true
Attachment #9090062 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9090062 [details] [diff] [review] Bug-1537729_remove_grid_msgHdrView-inc-xul.patch Review of attachment 9090062 [details] [diff] [review]: ----------------------------------------------------------------- From and subject are now close to each other, but then there is extra vertical space before To. But there's some more serious problem. Try with a small window, and the from and to fields (well the whole rows) are painted on top of each other. ::: mail/base/content/msgHdrView.js @@ +901,5 @@ > + if (!headerEntry.valid) { > + headerEntry.enclosingRow.setAttribute("hidden", "hidden"); > + } else { > + headerEntry.enclosingRow.removeAttribute("hidden"); > + } headerEntry.enclosingRow.toggleAttribute("hidden", !headerEntry.valid); @@ +953,1 @@ > } headerEntry.enclosingRow.toggleAttribute("hidden", !headerEntry.valid)
Attachment #9090062 - Flags: feedback?(mkmelin+mozilla) → feedback-
Attachment #9090062 - Attachment is obsolete: true
Attachment #9091454 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9091454 [details] [diff] [review] Bug-1537729_remove_grid_msgHdrView-inc-xul.patch Review of attachment 9091454 [details] [diff] [review]: ----------------------------------------------------------------- The ths are bold, while on trunk subject etc are not. But a bigger problem: When having an email with more emails than can fit, and opening up that list, there's a huge (like 100px) vertical gab between From and Subject
Attachment #9091454 - Flags: review?(mkmelin+mozilla) → review-

There is an issue on Trunk. When you have multiple email addresses, reduce the size of the screen. Click on more and expand the window size. You will see a gap there. I will attach a SS.

Attachment #9091454 - Attachment is obsolete: true
Attachment #9091721 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9091721 [details] [diff] [review] Bug-1537729_remove_grid_msgHdrView-inc-xul.patch Review of attachment 9091721 [details] [diff] [review]: ----------------------------------------------------------------- The header and value(s) are not 100% aligned vertically (e.g. the Subject) Also seems on trunk for longer to fields, we just show one line + the (and 87 else). With this patch, a few lines are shown
Attachment #9091721 - Flags: review?(mkmelin+mozilla) → review-

I am not seeing this issues on Mac. Can you attach a SS for Linux?

I checked on Linux machine. Alignment issue was there and I have it resolved now but I am not able to reproduce the issue for longer "to" fields.
I am still seeing "3 more" label at the end. And also can you ask somebody to check on Windows machine if margin and paddings are correct there or not.

Please attach what you have.

Attached image msgHdrinc_macOS.png (deleted) —
Attached image msgHdrInc_Ubuntu.png (deleted) —

But please attach the patch for it.

It's the same patch with margin changes for Linux.

Attachment #9091721 - Attachment is obsolete: true
Attachment #9092919 - Flags: review?(mkmelin+mozilla)
Attachment #9092919 - Flags: review?(mkmelin+mozilla)

I am not able to reproduce following mozmill test failures on Local Machine with the updated patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c7764d4311886512154f32068d5bcb462a0b08c
You told that you are not seeing the "more", can you attach a screenshot, please? Also, can you run the mozmill tests?

UI wise it looks ok now.
I can reproduce the more issue still. The mail has around 80 recipients, and that is likely a key factor. Try it with enough recipients of various lengths...

For the test failure, in the test you probably instead need to wait for the mode to change (the click will cause the event that triggers that the toggle, but looks like we're checking the state directly now, and that can be before the click handling ran). https://searchfox.org/comm-central/rev/a2afcaf776f3bbff0fe6c2db4c4190942aadca12/mail/test/mozmill/message-header/test-message-header.js#473

I can't reproduce the test failure locally.

I find the solution for the test failures. But I am not able to reproduce the issue with "more" button. I tried with the 80 odd email addresses. Also in test-message-header mozmill tests, we have few test cases with 100 odd email addresses and it is working fine there also. I will attach the screenshot here. We have a mozmill test to check the behavior of the "more" button and it is passing.
Can you check after moving that particular email to another folder or something? and this problem is only for that particular mail or all the other mails?

There are other test cases with 100 odd emails.

The issue is still there. I'll see if I can get you a test case.

Comment on attachment 9094869 [details] [diff] [review] Bug-1537729_remove_grid_msgHdrView-inc-xul.patch Review of attachment 9094869 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin
Attachment #9094869 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c9c2af227b34
remove grid usage from msgHdrView.inc.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Regressions: 1652410
Regressions: 1680843
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: