Closed Bug 1094320 Opened 5 years ago Closed 5 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)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: azasypkin, Assigned: julienw)

References

Details

Attachments

(1 file)

54 bytes, text/x-github-pull-request
gmarty
: review+
Details | Review
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
Hey Wilson,

Could you please look into this?

Thanks!
Flags: needinfo?(wilsonpage)
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)
(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">&nbsp;‎(+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)
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.
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)
See Also: → 1096421
(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?
(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)
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 :) )
Attached file github PR
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
Flags: needinfo?(gmarty)
Comment on attachment 8525503 [details] [review]
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 on attachment 8525503 [details] [review]
github PR

Looks good to me.
I'm happy to see that component getting more stable!
Attachment #8525503 - Flags: review?(gmarty) → review+
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`.
Landed in gaia-header/master: cacec75985511965d5b3a4e2b199d1ca28cee5df

PR for the gaia change: https://github.com/mozilla-b2g/gaia/pull/26696
All green, landed on Gaia master: 2cb461dde5b9430ed87919e768de8c1cea503db6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1102362
You need to log in before you can comment on or make changes to this bug.