Bug 9583 - OPAC cart button display problem in Chromium
Summary: OPAC cart button display problem in Chromium
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-10 21:39 UTC by Katrin Fischer
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
screenshot (18.34 KB, image/png)
2013-02-10 21:39 UTC, Katrin Fischer
Details
Bug 9583 - OPAC cart button display problem in Chromium (2.12 KB, patch)
2013-02-11 13:29 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9583 - OPAC cart button display problem in Chromium (2.25 KB, patch)
2013-02-11 16:11 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9583 - OPAC cart button display problem in Chromium (2.40 KB, patch)
2013-02-12 07:32 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-02-10 21:39:34 UTC
Created attachment 15211 [details]
screenshot

In chromium the cart button is node wide enough for the icon and the text.
(see screenshot)

This appeared in Chromium 0.88.1 in Ubuntu.
In Firefox 18.0.2 on the same system the display is correct.
Comment 1 David Cook 2013-02-10 23:06:18 UTC
(In reply to comment #0)
> Created attachment 15211 [details]
> screenshot
> 
> In chromium the cart button is node wide enough for the icon and the text.
> (see screenshot)
> 
> This appeared in Chromium 0.88.1 in Ubuntu.
> In Firefox 18.0.2 on the same system the display is correct.

I've had the same problem with Chrome 24.0.1312.57 m on Windows Vista and Windows 8.

IE 9 and Firefox 18.0.2 were fine. 

Last I checked, 8 Feb 2013, 3.10.x was displaying the cart fine. It was just not working on the most recent master.

Using the Chrome dev tool, I noticed that one of the <i> elements in the cart span had a height of 18px (for reasons I cannot explain), and that seemed to be messing things up. If I deleted that element, it looked perfect again. 

The height doesn't seem to come from any CSS that I can see though, so I assume there was an update to the Javascript at some point recently, and that had an impact on Chrome/Chromium, although I'm not sure why. 

I wonder if this happened before or after the Jquery libraries were moved?
Comment 2 Bernardo Gonzalez Kriegel 2013-02-10 23:48:24 UTC
All is normal if you go back before Bug 9265 were pushed
(git reset --hard a8355ce), so is something on it.
Comment 3 David Cook 2013-02-11 00:25:35 UTC
(In reply to comment #2)
> All is normal if you go back before Bug 9265 were pushed
> (git reset --hard a8355ce), so is something on it.

Agreed, Bernardo. I found the same thing when I did some trial and error in master. 

When I change the DOCTYPE back to the XHTML transitional, the OPAC cart button displays correctly again. When I revert it back to the HTML5 short doctype, it creates the display problem again.

I tried the <!DOCTYPE html SYSTEM "about:legacy-compat"> doctype as well in case Chrome might be happier with that, but the display problem still occurs. 

In the "HTML5 differences from HTML4", I noticed the following bit of text: 

"The i element now represents a span of text in an alternate voice or mood, or otherwise offset from the normal prose in a manner indicating a different quality of text, such as a taxonomic designation, a technical term, an idiomatic phrase from another language, a thought, or a ship name in Western texts."

While I assumed this was more of a "policy" difference, I wonder if it's possible that Chrome treats the i element differently because of the different doctype?
Comment 4 Owen Leonard 2013-02-11 00:59:29 UTC
The extra markup added by JavaScript in doc-head-close.inc is extraneous and should have been removed when we changed the way the buttons are constructed.
Comment 5 David Cook 2013-02-11 02:45:01 UTC
(In reply to comment #4)
> The extra markup added by JavaScript in doc-head-close.inc is extraneous and
> should have been removed when we changed the way the buttons are constructed.

Owen, is the extra mark-up the 2 empty "i" elements, and 2 empty span elements (one with an id of "carticon")?

If so, I'll submit a patch to remove them.
Comment 6 Owen Leonard 2013-02-11 13:29:06 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2013-02-11 13:30:46 UTC
(In reply to comment #5)
> If so, I'll submit a patch to remove them.

Thanks David. I felt responsible for the bug, so I went ahead and submitted a patch.
Comment 8 Bernardo Gonzalez Kriegel 2013-02-11 16:11:23 UTC Comment hidden (obsolete)
Comment 9 David Cook 2013-02-11 22:40:06 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > If so, I'll submit a patch to remove them.
> 
> Thanks David. I felt responsible for the bug, so I went ahead and submitted
> a patch.

Thanks for the patch, Owen! Thanks to Bernardo too for such a fast turnaround time on the sign off!
Comment 10 Katrin Fischer 2013-02-12 07:32:41 UTC
Created attachment 15267 [details] [review]
Bug 9583 - OPAC cart button display problem in Chromium

Legacy markup in the script which generates the OPAC's Cart button
causes display problems in Chrome under the new HTML5 doctype.

This patch removes the extra markup, which has been unnecessary since
we changed the way the Cart and Lists buttons are displayed in Bug
7584.

To test, view the OPAC in several browsers, including a Chromium-based
browser (Specifically Chromium-based. It is not a WebKit-related bug).
The Cart button should display correctly with and without items in the
cart.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment: Works great, back to normal.
Tested in windows (chrome, firefox, opera) and linux (chrome, chromium)

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested with Chromium and Firefox in Ubuntu. No regression found and
display is correct now.
Comment 11 Jared Camins-Esakov 2013-02-12 13:41:10 UTC
This patch has been pushed to master.
Comment 12 Chris Cormack 2013-02-14 08:51:22 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.3 and 3.8.10