Closed
Bug 1094320
Opened 10 years ago
Closed 10 years ago
It's impossible to have rich HTML markup inside heading elements (h1, h2, h3, h4) if they defined inside gaia-header component
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: azasypkin, Assigned: julienw)
References
Details
Attachments
(1 file)
In bug 1080828 we use <bdi> elements inside <h1>, so that we have something like this: <gaia-header><h1><bdi>Bidirectional text</bdi></h1></gaia-header> At some point gaia-header changes this markup to: <gaia-header><h1>Bidirectional text</h1></gaia-header> After quick debugging it seems that culprit is [1]. [1] https://github.com/mozilla-b2g/gaia/blob/7918024c737c4570cacd784f267e28737ae05dea/shared/elements/gaia-header/gaia-header.js#L223
Reporter | ||
Comment 1•10 years ago
|
||
Hey Wilson, Could you please look into this? Thanks!
Flags: needinfo?(wilsonpage)
Comment 2•10 years ago
|
||
I don't think it will be possible to support nested elemenets inside <h1> in gaia-header. The text positioning logic is super complex, and relies on this assumption. Have you tried using CSS (unicode-bidi: isolate) [1]. [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/bdi
Flags: needinfo?(wilsonpage)
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #2) > I don't think it will be possible to support nested elemenets inside <h1> in > gaia-header. The text positioning logic is super complex, and relies on this > assumption. > > Have you tried using CSS (unicode-bidi: isolate) [1]. > > [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/bdi Our real use case is a bit more complex than one I mentioned in comment 0: <gaia-header> <h1> <bdi>Localizable_Term</bdi> <span class="some-class" dir="ltr"> (+3)</span> </h1> </gaia-header> So probably we can workaround "some-class" logic, not sure about bidirectional content inside, we'll see if we can use left-to-right mark here instead (though using marks looks like a hack to me). Julien, do you have any other ideas how to solve this?
Flags: needinfo?(felash)
Reporter | ||
Comment 4•10 years ago
|
||
Unfortunately it seems impossible to achieve effect we want with LTR and RTL marks only. See "FirstName LastName (+1)" example in bug 1080828 comment 3 to understand what we're trying to display in the heading. To display it correctly we need to somehow add dir="auto" for the "FirstName LastName" part.
Assignee | ||
Comment 5•10 years ago
|
||
Hey Wilson, can you explain why you just don't call runFontFit (or even fontFit.reformatHeading) in rerunFontFit? The font fit algorithm works fine for us even with nested elements (obviously they need to be inline and without fancy css markup), it's only this line that flattens the elements.
Flags: needinfo?(felash) → needinfo?(wilsonpage)
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #5) > Hey Wilson, can you explain why you just don't call runFontFit (or even > fontFit.reformatHeading) in rerunFontFit? > > The font fit algorithm works fine for us even with nested elements > (obviously they need to be inline and without fancy css markup), it's only > this line that flattens the elements. Ok, forget what I say here, it doesn't work fine with nested elements currently. Maybe it's possible to make it work with inline nested elements though?
Comment 7•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #5) > Hey Wilson, can you explain why you just don't call runFontFit (or even > fontFit.reformatHeading) in rerunFontFit? > > The font fit algorithm works fine for us even with nested elements > (obviously they need to be inline and without fancy css markup), it's only > this line that flattens the elements. I understand the problem you're facing now :) We don't have a public `.runFontFit()` method currently. I think we do need it due to the number of cases in Gaia that require this feature. I remember :gmarty had issues with us exposing this, IMO the mutation observers just aren't good enough solution, and calling them via reseting `textContent` is a hacky solution that I'm forever having to explain to people :( NI :gmarty to help us come to a solution.
Flags: needinfo?(wilsonpage) → needinfo?(gmarty)
Assignee | ||
Comment 8•10 years ago
|
||
Note that I removed the textContent reseting line as part of my WIP patch in https://github.com/mozilla-b2g/gaia/pull/26194, it seems to work fine (but nested markups still don't work properly and I still have other issues though ;) ). I see "runFontFit" is called directly in the music application, do you know if it's been done on purpose? (I think you're actually the author of the patch, Wilson :) )
Assignee | ||
Comment 9•10 years ago
|
||
I looked with Guillaume and we found the issue. So it looks like we have a lot of spaces in the second <bdi> element, and gaia-header does not trim or squash the spaces. So the patch does this and change the "textContent = textContent" call to a direct call to reformatHeading. Unit tests are not finished yet. We should check why we have so much spaces in this element though.
Assignee: nobody → felash
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(gmarty)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8525503 [details]
github PR
Hey Guillaume, I think it's ready!
Maybe you'd prefer that I ask the review to Wilson?
Attachment #8525503 -
Flags: review?(gmarty)
Comment 11•10 years ago
|
||
Comment on attachment 8525503 [details]
github PR
Looks good to me.
I'm happy to see that component getting more stable!
Attachment #8525503 -
Flags: review?(gmarty) → review+
Assignee | ||
Comment 12•10 years ago
|
||
next steps: To stamp a new version: `bower version patch && git push upstream master --tags` To land in Gaia: We land to gaia-components, stamp a version and in Gaia `bower update gaia-header`.
Assignee | ||
Comment 13•10 years ago
|
||
Landed in gaia-header/master: cacec75985511965d5b3a4e2b199d1ca28cee5df PR for the gaia change: https://github.com/mozilla-b2g/gaia/pull/26696
Assignee | ||
Comment 14•10 years ago
|
||
All green, landed on Gaia master: 2cb461dde5b9430ed87919e768de8c1cea503db6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•