Closed Bug 1171925 Opened 5 years ago Closed 5 years ago

Clicking the title or favicon for context (in the conversation/standalone windows) should appear to be part of the link and open the webpage

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
2

Tracking

(firefox41 affected, firefox42 affected, firefox43 verified)

VERIFIED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox41 --- affected
firefox42 --- affected
firefox43 --- verified

People

(Reporter: bogdan_maris, Assigned: standard8)

References

Details

(Whiteboard: [context])

Attachments

(6 files, 4 obsolete files)

3.62 MB, image/gif
Details
31.45 KB, image/png
sevaan
: ui-review-
Details
33.92 KB, image/png
sevaan
: ui-review+
Details
46.02 KB, image/png
sevaan
: ui-review+
Details
42.78 KB, image/png
sevaan
: ui-review+
Details
31.59 KB, patch
dmose
: review+
Details | Diff | Splinter Review
Attached image Gif showing the issue
Affected builds:
- Latest Nightly 41.0a1

Affected OS`s:
- Windows 7 64-bit
- Mac OS X 10.9.5
- Ubuntu 14.04 32-bit

STR:
1. Start Firefox
2. Click Hello icon
3. Check 'Let`s talk about' and start a Conversation
4. Click the name and favicon of the website added in context

Expected results: Nothing happens.

Actual results: Shared website is loaded in new tab.

Notes:
- Gif with the issue attached.
- This is not a regression, it reproduces after context layout changes in bug 1162909.
Whiteboard: [context]
sevaan - is this what we want it to do?  i think so... but ....
Flags: needinfo?(sfranks)
Yes, this gives a larger target for users to click on.

However, the URL should be underlined even when a user mouses over the favicon or the title. This makes it clearer that the entire area is one link, rather than each element being a link.
Flags: needinfo?(sfranks)
After moving context to chat area (fix from bug 1171940), clicking the title or favicon will not redirect to the url anymore. Sevaan is this the intended behavior? If so, I can close this bug as Invalid.
Flags: needinfo?(sfranks)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #3)
> After moving context to chat area (fix from bug 1171940), clicking the title
> or favicon will not redirect to the url anymore. Sevaan is this the intended
> behavior? If so, I can close this bug as Invalid.

No, those links should still take you to the URL.
Flags: needinfo?(sfranks)
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P1
Updating the title to reflect what this bug is really about.
Summary: Name and favicon from conversation window are redirecting to shared webpage in context → Clicking the title or favicon for context (in the conversation/standalone windows) should open the webpage
Summary: Clicking the title or favicon for context (in the conversation/standalone windows) should open the webpage → Clicking the title or favicon for context (in the conversation/standalone windows) should appear to be part of the link and open the webpage
The UI on Nightly has changed since this bug was filed.  Need to review the current user-experience with Sevaan to see exactly how he wants it changed, if he does indeed want an analogous change.  Sevaan, can you try the steps-to-reproduce on a current nightly build and let us know what you'd like?
Flags: needinfo?(sfranks)
Just tried it.

- Link should underline only on over
- Entire area should be clickable and open link in new active tab.
- Probably link cursor, not arrow cursor
- Can we limit the link title to two lines, and roll it off into ellipses if it goes long? This doesn't look great, especially since it starts scrolling: http://i.sevaan.com/image/2M0r2t202h0z
Flags: needinfo?(sfranks)
(In reply to Sevaan Franks [:sevaan] from comment #7)
> Just tried it.
> 
> - Link should underline only on over
> - Entire area should be clickable and open link in new active tab.
> - Probably link cursor, not arrow cursor

I assume you mean the link cursor for the entire area, not just for the link itself?

> - Can we limit the link title to two lines, and roll it off into ellipses if
> it goes long? This doesn't look great, especially since it starts scrolling:
> http://i.sevaan.com/image/2M0r2t202h0z

Sounds doable.
Flags: needinfo?(sfranks)
> I assume you mean the link cursor for the entire area, not just for the link
> itself?

Correct.
Flags: needinfo?(sfranks)
Assignee: nobody → standard8
Sevaan, I've done the majority of what you asked for, there's a few assumptions I'm making, and also a question or two:

- I'm assuming in the panel, we don't want the context to be highlighted on hover (since it doesn't make sense for it to be clickable).
- The best we can do for the description is to make it one line with an ellipsis. Doing two-line is difficult for various reasons. Is that ok, or do you want something else?
- If we do the one-line with ellipsis, then we'd set a tooltip with the full description. This will be available even when the text doesn't overflow (possibly slightly better for accessibility).
- If we're going to have the one-line with ellipsis, do you just want that in the small conversation window, i.e. should expanded view / standalone / panel all have the full text?
Attachment #8644292 - Flags: ui-review?(sfranks)
Iteration: --- → 42.3 - Aug 10
Rank: 19
Comment on attachment 8644292 [details]
Screenshot of adjusted view

(In reply to Mark Banner (:standard8) from comment #10)
> Created attachment 8644292 [details]

> - I'm assuming in the panel, we don't want the context to be highlighted on
> hover (since it doesn't make sense for it to be clickable).

Correct.

> - The best we can do for the description is to make it one line with an
> ellipsis. Doing two-line is difficult for various reasons. Is that ok, or do
> you want something else?

Are you talking about the page title here, or the comment you can add in the Edit Context menu? If it's the page title, one line is a no go, as almost every page title will trail off to ellipses. I think I'd rather no limits than too short a limit.

If it's the contextual tile and the title is really long, let's keep the user scrolled at the top of the chat list when they join, rather than at the bottom of the list, cutting off the top of the tile. I'd rather then scroll down, then have to go up first.

Is this possible?

How difficult is a character limit that would reduce everything to two lines?

Could this be cheated in anyway? Maybe a fixed height div with no overflow so any extra text just gets hidden? But then we'd lose the ellipses...I don't know...

Maybe we need to rethink these tiles completely.
Attachment #8644292 - Flags: ui-review?(sfranks) → ui-review-
Attached image Alternative option
I took a look at multi-line ellipsis the other day, and there may be one or two libraries about, but it does look quite difficult.

I think the solution you've suggested is reasonable though now I've tried it out.

So here's the full list of changes, I'm making:

1) Going anywhere within the rectangle of the context will show the blue background and underline the url.
2) Clicking anywhere within the rectangle will open the url in a new tab.
3) The panel won't show the hover effects.
4) Stopped the automatic scroll if there's just context in the list.
5) Removed the useless whitespace that we were getting that was highlighted when we had context with multiple lines (the text chat area was unnecessarily expanding even though the entries was its correct size).
6) Fix the "Type Here..." message to be shown only when the user has not sent messages. Currently it is being hidden when the user had received messages (or in the case of desktop, if the user had context in the room).

The screenshot here is of the multiple line without scroll.

I'll attach a screenshot of the change for 5) in a moment.
Attachment #8644947 - Flags: ui-review?(sfranks)
This is the reduction of space that text chat is using. I think this was actually a regression from my media layout work that I hadn't spotted.
Attachment #8644948 - Flags: ui-review?(sfranks)
See comment 12/13 for what this fixes. The standalone-* removals in conversation.css were just some old classes I found that were unused.

I think overall this gets us working a bit better and nicer.
Attachment #8644977 - Flags: review?(dmose)
Comment on attachment 8644948 [details]
Text chat taking up less space

Great to lose that white box.

Can we correct the padding in the blue area so that there is equal padding all around, and that it all lines up with the edge of the T in Type here? It looks like 6px to me.

Not sure if this is the bug for it. But +r'ing for the whitespace removal.
Attachment #8644948 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8644947 [details]
Alternative option

+r, but also the padding issue. The Let's talk about is slightly too close to the video window above.
Attachment #8644947 - Flags: ui-review?(sfranks) → ui-review+
Ok, this should be much better I've added a consistent padding around. It does make the text boxes slightly offset, but otherwise I think we'll have to go for too much of a big padding - but maybe you think different.
Attachment #8645670 - Flags: ui-review?(sfranks)
Updated for ui-review
Attachment #8644977 - Attachment is obsolete: true
Attachment #8645670 - Attachment is obsolete: true
Attachment #8644977 - Flags: review?(dmose)
Attachment #8645670 - Flags: ui-review?(sfranks)
Attachment #8645671 - Flags: review?(dmose)
Comment on attachment 8645670 [details]
Updated with better padding

Make this attachment right...
Attachment #8645670 - Attachment is obsolete: false
Attachment #8645670 - Attachment is patch: false
Attachment #8645670 - Attachment mime type: text/plain → image/png
Attachment #8645670 - Flags: ui-review?(sfranks)
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
More UX comment addressing.
Attachment #8645671 - Attachment is obsolete: true
Attachment #8645671 - Flags: review?(dmose)
Attachment #8645830 - Flags: review?(dmose)
Comment on attachment 8645670 [details]
Updated with better padding

+R, but let's match the padding so that the vertical lines of the context tile match the vertical lines of the speech bubbles. Thanks!
Attachment #8645670 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8645830 [details] [diff] [review]
Allow the entire area of Loop's context to be clicked; don't show hover effects in the panel.

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

Looking good, I'd like to see the scrolling tests before this lands.  Thanks for putting this together!

::: browser/components/loop/content/shared/css/common.css
@@ +549,5 @@
>    /* Use the flex row mode to position the elements next to each other. */
>    display: flex;
>    flex-flow: row nowrap;
>    line-height: 1.1em;
> +  text-decoration: none;

A little bit of testing suggests to me that this rule isn't actually having any effect (even in the panel view).  If I'm wrong, a comment describing what effect this is supposed to have be great, otherwise, please remove.

@@ +589,5 @@
> +}
> +
> +:not(.no-click).context-wrapper:hover > .context-info > .context-url {
> +  text-decoration: underline;
> +}

A comment or two describing why these two rules are desirable would likely be helpful to future code maintainers.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1458,5 @@
>    margin-left: .2em;
>  }
>  
> +/* If you change this entry, check it doesn't affect the "special" text
> +   chat entries as well */

Slightly more detail about where to find out about the special text chat entries would be helpful to folks who haven't touched that code before.

@@ +1740,4 @@
>      height: auto !important;
> +    /* Let the view be the minimum size it needs to be - don't flex to take up
> +       more. */
> +    flex: 0 0 auto !important;

From what I can tell, neither the height nor the flex need !important any more, as they're not conflicting with any other rules that I can see.  Remove these !importants and the comment?

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +147,5 @@
>  
>      componentDidUpdate: function() {
> +      // Don't scroll if we haven't got any chat messages yet - e.g. for context
> +      // display, we want to display starting at the top.
> +      if (this.shouldScroll && this._hasChatMessages()) {

An automated test or two around the scrolling behavior seems like it would be a good thing.

::: browser/components/loop/test/shared/views_test.js
@@ +849,5 @@
>        return TestUtils.renderIntoDocument(
>          React.createElement(sharedViews.ContextUrlView, props));
>      }
>  
> +    it("should set a no-click class if clicks are not allowed", function() {

I think that naming this in the positive (eg .clicks-allowed) would make for more readable code & CSS (eg avoids double-negatives in the CSS, which are harder to think about).  I'll leave it up to you whether you want to do this or not.
Attachment #8645830 - Flags: review?(dmose) → feedback+
Comment on attachment 8645670 [details]
Updated with better padding

The input area placeholder text is much bigger than the one in the bubbles and the rest of the UI, can you make it the same size as the text inside the chat bubbles? 
I think it is 12px.

Also, (referring to image in the right) the gap between the light blue background and the gray border of the input area does not make sense, why not extend the background until reaching the border?

Thanks.
(In reply to Victoria Gerchinhoren [:vicky] from comment #23)
> The input area placeholder text is much bigger than the one in the bubbles
> and the rest of the UI, can you make it the same size as the text inside the
> chat bubbles? 
> I think it is 12px.

I believe the existing height is to accommodate the link/emoticon icons that will exist to the right of the text box.

We can change that, but we need an updated UX spec with the new design, and please can this be on a new bug - this one is already fixing too much.

> Also, (referring to image in the right) the gap between the light blue
> background and the gray border of the input area does not make sense, why
> not extend the background until reaching the border?

If there's multiple lines of text in the title of the url, that gap will disappear due to the overflow. I think trying to make it go away for the single line case is likely to be fragile, and arguably, more confusing to the user for when there are items in the list and they are viewing the top of the scroll section.
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #22)
> ::: browser/components/loop/content/shared/css/common.css
> @@ +549,5 @@
> >    line-height: 1.1em;
> > +  text-decoration: none;
> 
> A little bit of testing suggests to me that this rule isn't actually having
> any effect (even in the panel view).  If I'm wrong, a comment describing
> what effect this is supposed to have be great, otherwise, please remove.

The panel isn't the issue. Its the context in the conversation window. I've added a comment, but I really think its just describing what text-decoration: none is.

> > +:not(.no-click).context-wrapper:hover > .context-info > .context-url {
> > +  text-decoration: underline;
> > +}
> 
> A comment or two describing why these two rules are desirable would likely
> be helpful to future code maintainers.

Again, I'm confused. A comment seems to be along the lines of, when hovering change the display - imo you can get that by reading the css or simple experimentation.

> @@ +1740,4 @@
> >      height: auto !important;
> > +    /* Let the view be the minimum size it needs to be - don't flex to take up
> > +       more. */
> > +    flex: 0 0 auto !important;
> 
> From what I can tell, neither the height nor the flex need !important any
> more, as they're not conflicting with any other rules that I can see. 
> Remove these !importants and the comment?

Its needed in both cases for the reasons specified in the comment. I'm not sure precisely which entries off-hand, but things like:

.desktop-room-wrapper > .media-layout > .media-wrapper > .text-chat-view

higher up in the file, actually override this.
Updated for review comments.
Attachment #8646456 - Flags: review?(dmose)
Duplicate of this bug: 1190809
Attachment #8645830 - Attachment is obsolete: true
Updated to latest fx-team
Attachment #8646456 - Attachment is obsolete: true
Attachment #8646456 - Flags: review?(dmose)
Attachment #8646556 - Flags: review?(dmose)
Comment on attachment 8646556 [details] [diff] [review]
Allow the entire area of Loop's context to be clicked; don't show hover effects in the panel.

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

r=dmose
Attachment #8646556 - Flags: review?(dmose) → review+
Comment on attachment 8646556 [details] [diff] [review]
Allow the entire area of Loop's context to be clicked; don't show hover effects in the panel.

>+        // If we're running code coverage in Karma, we might not have
>+        // a fixtures element already.
>+        if (!fixtures) {
>+          fixtures = document.body.appendChild(document.createElement("div"));
>+          fixtures.id = "fixtures";
FYI, I added a line to karma head.js to auto-create div#fixtures:

https://hg.mozilla.org/integration/fx-team/file/09feeb70edac/browser/components/loop/test/karma/head.js#l10
https://hg.mozilla.org/mozilla-central/rev/7726e996f847
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(In reply to Mark Banner (:standard8) from comment #12)
> So here's the full list of changes, I'm making:
> 
> 1) Going anywhere within the rectangle of the context will show the blue
> background and underline the url.
> 2) Clicking anywhere within the rectangle will open the url in a new tab.
> 3) The panel won't show the hover effects.
> 4) Stopped the automatic scroll if there's just context in the list.
> 5) Removed the useless whitespace that we were getting that was highlighted
> when we had context with multiple lines (the text chat area was
> unnecessarily expanding even though the entries was its correct size).
> 6) Fix the "Type Here..." message to be shown only when the user has not
> sent messages. Currently it is being hidden when the user had received
> messages (or in the case of desktop, if the user had context in the room).

Verified all this changes using latest Nightly 43.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit). Keeping qe-verify+ for FF 42 and 41, when the fix will arrive there.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.