Closed Bug 936524 Opened 7 years ago Closed 7 years ago

Tag names with blanks causes strange message pane summary behavior

Categories

(Thunderbird :: Mail Window Front End, defect)

24 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 28.0

People

(Reporter: erik.krause, Assigned: alta88)

References

Details

Attachments

(4 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

Tag a message in a thread with a tag name containing a blank (f.e. german "Zu erledigen"). Create such a tag if there is none. Collapse all message threads, then select the thread with the tagged message. Eventually select another thread, then the one with the tagged message again.


Actual results:

Message pane displays the start message of the thread or nothing at all (grey). 


Expected results:

Message pane should display a summary of all selected messages (the thread).
Severity: normal → minor
Erik, can you attach a screenshot pls?
Flags: needinfo?(erik.krause)
Flags: needinfo?(erik.krause)
Further, there is an error in console:
Error: String contains an invalid character = InvalidCharacterError

This (likely) is due to creating a <span> element to contain a tag in a button-like decoration/color in the html document, and attempting to assign a class value with a value containing a space.

There isn't really any reason to add the name of the tag to the class; what matters for css is 'tag' and 'blc-FF6666' (for example, a color).
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Duplicate of this bug: 921481
Attached patch tag.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8342158 - Flags: review?(mkmelin+mozilla)
Attached patch tag.patch (obsolete) — Splinter Review
may as well add the tag name as an attribute.
Attachment #8342158 - Attachment is obsolete: true
Attachment #8342158 - Flags: review?(mkmelin+mozilla)
Attachment #8342162 - Flags: review?(mkmelin+mozilla)
Do we really need to add it as an attribute if the textContent already holds the tag name? It's not clear that it's used anywhere.

Also, the issue appears twice in selectionsummaries.js, so you'll need to fix the other occurrence as well.
Attached patch tag.patch (obsolete) — Splinter Review
i think we should add the attr, as otherwise a particular tag can't be css selected.  that was undoubtedly the purpose of putting it in the class to begin with.
Attachment #8342162 - Attachment is obsolete: true
Attachment #8342162 - Flags: review?(mkmelin+mozilla)
Attachment #8342325 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8342325 [details] [diff] [review]
tag.patch

Review of attachment 8342325 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/selectionsummaries.js
@@ +364,5 @@
>          let tagNode = tagsNode.ownerDocument.createElement('span');
>          // see tagColors.css
>          let colorClass = "blc-" + this._msgTagService.getColorForKey(tag.key).substr(1);
> +        _mm_addClass(tagNode, ["tag", colorClass]);
> +        tagNode.setAttribute("tag", tag.tag);

If we want to do this, I think we should use

  tagNode.dataset.tag = tag.tag;

since that's a more standard way of setting custom attributes in HTML5. This *should* work in XHTML too...

@@ +369,1 @@
>          tagNode.textContent = tag.tag;

We can remove this and use CSS to show the tag's name:

  tag::after {
    content: attr(data-tag);
  }
(In reply to Jim Porter (:squib) from comment #11)
> We can remove this and use CSS to show the tag's name:
> 
>   tag::after {
>     content: attr(data-tag);
>   }

I guess this would make it so you can't copy the tag name, but maybe we don't want that to happen anyway...
> 
>   tagNode.dataset.tag = tag.tag;
> 

sure.

(In reply to Jim Porter (:squib) from comment #12)
> (In reply to Jim Porter (:squib) from comment #11)
> > We can remove this and use CSS to show the tag's name:
> > 
> >   tag::after {
> >     content: attr(data-tag);
> >   }
> 
> I guess this would make it so you can't copy the tag name, but maybe we
> don't want that to happen anyway...

it would be quite odd to remove select/copy from an html page for this use case, like a regression to xul-land.
Attached patch tag.patchSplinter Review
Attachment #8342325 - Attachment is obsolete: true
Attachment #8342325 - Flags: review?(mkmelin+mozilla)
Attachment #8343115 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 8343115 [details] [diff] [review]
tag.patch

Review of attachment 8343115 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Thanks! rs=me
Attachment #8343115 - Flags: review?(squibblyflabbetydoo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/86eef5c65d21
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
(we should get fixed bugs out of Untriaged)
Component: Untriaged → Mail Window Front End
Duplicate of this bug: 920489
You need to log in before you can comment on or make changes to this bug.