757257 - Ubi.com: irregular layout on search results

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Gregory Jost]

Attachment: Correct: consistent listing style (Chrome)

Web page or screen you were on when you saw the issue: 
http://www.ubi.com/US/games/search.aspx?cal=nreleases
Listing layout is inconsistent.


Steps to reproduce:
1. go to ubi.com/US
2. click "games" section in navigation and selec "new releases"
3. listing displays incorrectly

What you expected: 
consistent font and display across all list items

Comment 1

[Gregory Jost]

Attachment: Incorrect: inconsistent listing display (FF)

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Looks similar to the ycombinator issue. bug 707195

Comment 3

[Jet Villegas (inactive)]

P3 per font inflation scrub.

Comment 4

[Scott Johnson (:jwir3)]

These appear to be using floats, which is a similar case to the tables in bug 707195, but not identical. I think it is dependent, though.

Comment 5

[Scott Johnson (:jwir3)]

One thing that I've noticed about this page since the font inflation triage that kats and I have done is that we have a structure of the page similar to this:

       <div>
       /   \
      /     \ 
   <div>    <div>
    /         \
  <div>      <div>
   /            \
 <div>        <div>
 /               \
<p>      ...     <p>

Perhaps this type of situation could be easily eliminated if we had items that were at the same level in the content tree have the same font inflation, IF they have an equivalent style.

Comment 6

[Scott Johnson (:jwir3)]

BTW, the tree in comment 5 was supposed to represent the position of two paragraph elements (one being inflated currently, and one not), which, in the case of the ubi.com site, represents paragraphs that we want to inflate consistently.

Comment 7

[Scott Johnson (:jwir3)]

Kats is working on an experiment that might help solve this issue.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

As a braindump, my thinking here is as follows:

The search results on ubi.com are inflated individually, and some of them are too short to get inflated (this can be seen by changing the font.size.inflation.lineThreshold to 0, which fixes the problem here). My idea is to implement some sort of block-similarity heuristic, which identifies that the different search results should be inflated together. Then, if any one of the results is inflated, they will all be inflated. In pseudocode this might look like:

really_inflate(block) {
  if (inflate(block)) return true;
  for (similar_block : get_similar_blocks(block)) {
    if (inflate(similar_block)) return true;
  }
  return false;
}

where "inflate" is the existing heuristic for inflation and "get_similar_blocks" is the new heuristic I'm proposing.

If we implement this behaviour and leave the lineThreshold as-is, it should fix this page by inflating all of the search results by the same amount. It should also not regress bug 706193 because even though the heuristic will identify the nytimes.com footer pieces as similar, none of the footer pieces exceed the lineThreshold and so none of them will be inflated.

Comment 9

[Scott Johnson (:jwir3)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> As a braindump, my thinking here is as follows:
> 
> The search results on ubi.com are inflated individually, and some of them
> are too short to get inflated (this can be seen by changing the
> font.size.inflation.lineThreshold to 0, which fixes the problem here). My
> idea is to implement some sort of block-similarity heuristic, which
> identifies that the different search results should be inflated together.
> Then, if any one of the results is inflated, they will all be inflated. In
> pseudocode this might look like:
> 
> really_inflate(block) {
>   if (inflate(block)) return true;
>   for (similar_block : get_similar_blocks(block)) {
>     if (inflate(similar_block)) return true;
>   }
>   return false;
> }
> 
> where "inflate" is the existing heuristic for inflation and
> "get_similar_blocks" is the new heuristic I'm proposing.
> 
> If we implement this behaviour and leave the lineThreshold as-is, it should
> fix this page by inflating all of the search results by the same amount. It
> should also not regress bug 706193 because even though the heuristic will
> identify the nytimes.com footer pieces as similar, none of the footer pieces
> exceed the lineThreshold and so none of them will be inflated.

I think this is a good idea. One comment, I have though -

We need to be careful about how we inflate previously "similar" blocks. For example, consider going through a reflow iteration, down the frame tree. If we have a frame tree corresponding to the content tree I gave in comment 5, so it looked like:

         <root frame>
       /             \
      /               \ 
   <f1>              <f2>
    /                   \
  <f3>                 <f4>
   /                      \
 <f5>                    <f6>
 /                         \
<f7>   ... <f{7+k}> ...   <f{7+n}>

(And we assume that <f7>, <f{7+k}>, <f{7+n}>, where n > k are all "similar" block frames, then if we make a negative inflation decision about <f7>, but, later in reflow, make a positive inflation decision about <f{7+k}> or <f{7+n}>, then how can we efficiently go back and re-inflate these frames?

It seems like we'll need some type of mechanism to do this efficiently, since we could end up interrupting reflow multiple times and retriggering it.

Does this make sense?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Scott Johnson (:jwir3) from comment #9)
> Does this make sense?

Yup, that makes perfect sense. And thanks for bringing it up, I hadn't really considered that scenario. I guess in the best case we would end up triggering a second reflow. Once the "dirty" frame tree has been completely reflowed, if it inflated things that were previously not inflated, AND if those things have other similar blocks that should now be inflated, we would have to trigger the second reflow from the top. I don't know how often that would happen but it's worth keeping in mind for sure.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

So this is what I thought would work, but doesn't seem to, and I'm not sure why. Basically what I'm doing is grouping together elements that share the same nsRuleNode, because those are the ones that match the same set of style rules. I store the minimum inflation font size on the rule node and reuse it whenever another element is encountered that has the same rule node (this is just a quick hack that can be cleaned up later). I have a logging patch that I will attach as well, when I load http://www.ubi.com/US/games/search.aspx?cal=nreleases with this applied, I see from the logging that it's doing what I expect - that is, once it encounters a <p> elements inside the game detail info blocks that *is* inflated, it applies the same inflation to all the other <p> elements, even if their inflation data is inflation should not be enabled because they are too short.

But.. the page still shows up exactly the same. Is there some other hook that is controlling how much the blocks get inflated? I assumed that returning the right value from InflationMinFontSizeFor would be sufficient, but maybe not?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP debugging patch

This is what I used to verify that what's happening is what I expect to happen. The output is non trivial to wade through, but I can walk you through that if needed.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP that works

Huzzah! :jwir3 pointed me to http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#2091 which is where the inflation value is actually assigned to a text run. Turns out the code there passes in a text frame and when I wrote my code I got mixed up between the frame passed in and the font inflation container which is an element. This updated patch does what I expect.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

This also works beautifully on news.ycombinator.com. I still need to handle the case :jwir3 described in comment #9 (and a closely related problem, which is that on the first layout pass, we do layout of a small piece of text and then later inflate a larger piece of text that is grouped with the small piece, but never go back to re-do the small piece). It doesn't look like we'll hit these case very often in practice because it seems like layout visits a lot of nodes any time it does a reflow, but still need to at least detect when that happens.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 702458 [details] [diff] [review]
WIP that works

Requesting for any feedback you might have on this. I pushed a try build with this patch to https://tbpl.mozilla.org/?tree=Try&rev=263f42203139 so you can try it out once it's done.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 702458 [details] [diff] [review]
WIP that works

First some comments on the code details (rather than the algorithm, which I need to think about more).

The way you're getting a style context is *way* too complicated.  Frames already have pointers to style contexts; the code you're calling is for code that only has a DOM element and wants a style context (even if the DOM element has no frame).  You want to remove this:

>+#include "nsComputedDOMStyle.h"
>+

And replace this:

>+
>+      nsRuleNode* ruleNode = NULL;
>+      nscoord value;
>+      if (f->GetContent() && f->GetContent()->IsElement()) {
>+        nsRefPtr<nsStyleContext> style = nsComputedDOMStyle::GetStyleContextForElementNoFlush(f->GetContent()->AsElement(), NULL, f->PresContext()->GetPresShell(), nsComputedDOMStyle::eAll);
>+        ruleNode = style->GetRuleNode();

with just:

+      nsRuleNode *ruleNode = nullptr;
+      if (f->GetContent() && f->GetContent()->IsElement()) {
+        ruleNode = f->GetStyleContext()->GetRuleNode();

(since I suspect you probably do still want the IsElement() check, but I'm not sure).




>+        if (ruleNode->mMinInflatedFontSize > 0) {
>+          // if (data && !data->InflationEnabled()) here, then this aFrame is set to
>+          // not be inflated by default, but we are overriding it to be inflated
>+          // based on grouping with other elements that share the same ruleNode.
>+          return ruleNode->mMinInflatedFontSize;
>+        }
>+      }
>+
>       // FIXME: The need to null-check here is sort of a bug, and might
>       // lead to incorrect results.
>       if (!data || !data->InflationEnabled()) {
>         return 0;
>       }
> 
>-      return MinimumFontSizeFor(aFrame->PresContext(),
>+      value = MinimumFontSizeFor(aFrame->PresContext(),
>                                 data->EffectiveWidth());

Here, declare value at first use (rather tan above where in the chunk I said to remove).





That said, I also don't think this algorithm makes sense.  It looks to me like you're doing the calculation based on the width of the first frame you encounter that happens to have a particular rule node (i.e., happens to match a particular set of CSS rules).  That's going to lead to very strange results (e.g., adding an element earlier in a narrower container that happens to match the same rules as later elements can cause drastic changes) and also violates a bunch of other reasonable expectations, including the concept that the layout after DOM or style mutations is the same layout you'd get loading the resulting DOM as a static page.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Baron [:dbaron] from comment #17)
> The way you're getting a style context is *way* too complicated.

Thanks, I obtained that code mostly by looking through addons that I knew did similar things and figuring out what they were calling. I can switch it to the simpler version you posted.

> That said, I also don't think this algorithm makes sense.  It looks to me
> like you're doing the calculation based on the width of the first frame you
> encounter that happens to have a particular rule node (i.e., happens to
> match a particular set of CSS rules).  That's going to lead to very strange
> results (e.g., adding an element earlier in a narrower container that
> happens to match the same rules as later elements can cause drastic changes)

So you're right that my WIP currently has this problem, but I was expecting to be able to fix that. Actually there's a few different semi-related problems that I want to discuss.

Problem the first: Simple node ordering
---------------------------------------
I have two test pages:

http://people.mozilla.com/~kgupta/bug/757257.html
http://people.mozilla.com/~kgupta/bug/757257-2.html

The first one is a case where the current font inflation code inconsistently inflates the two paragraphs, and my patch fixes by "copying" the font inflation from the first paragraph onto the second paragraph. The second test page reverses the order of the two paragraphs. Since the first, shorter paragraph is not ordinarily inflated, I would expect this page to render with only the second paragraph inflated. However, my patch (for some reason that I do not yet know) does cause both to be inflated. I suspect (but have not verified) that we just end up running the flow algorithm on those nodes more than once and so when we flow the top paragraph for the second time it gets inflated.

Therefore this has not been a problem in practice, but is still one in theory. The way I plan to solve this node ordering problem is by somehow associating elements with the nsRuleNode such that when the nsRuleNode->mMinInflatedFontSize is changed (which should happen only once), it will mark for reflow all the elements that were previously associated with it.

Problem the second: Dynamic updates
-----------------------------------
This is the problem where we have some page that is laid out and then mutated dynamically. Lets say that there are two element "groups" - element group A which all share rule node rA and are all inflated, and element group B which all share rule node rB and are all not inflated. New elements added to group A will always be inflated, but adding elements to group B may require inflating all of group B. The opposite case also needs to be handled, where removing elements from group A may require un-inflating all of group A, but removing elements from group B will not change the rest of group B.

I expect to be able to implement this using the same nsRuleNode ->* element mapping that I mentioned in the previous problem's solution. I expect it to be non-trivial to do this while avoiding unnecessary reflows, but it should be possible.

Problem the third: Varying inflation amounts
--------------------------------------------
Thus far I have been making a simplifying assumption, which is that for each element in a "group" (as per my patch), the element is either not inflated at all, or is inflated to the same amount. This assumption is false, and I think what you were referring to comment #17. That is, we could have two elements A and B that share the same nsRuleNode, but whose actual inflation value depends on other factors like the parent frames, and so are inflated differently. My current patch will simply pick the inflation amount from the first encountered frame and apply this to all of them. This can result in non-deterministic behaviour from the web developer's point of view because reordering pieces of the page will change how much things get inflated.

The first option to dealing with this problem is to not use my algorithm in this case. That is, if we detect a "group" of elements where there are two or more elements with different inflation amounts, we disband the group and don't do any copying of inflation. This could be implemented using the same nsRuleNode ->* element mapping described above, and by clearing the mapping upon disbanding the group.

Another option is to use a different operator when copying inflation amounts. For example, we could take the different inflation amounts of all the elements in the group, and use the max() of all of them. Or perhaps the average(). (You could say my WIP uses first()). This should solve the problem for the web developer in that rearranging the elements doesn't change behaviour. Adding/removing elements may still change behaviour though. Implementing this may require reflowing all of the elements in a group any time any of the elements is modified, and so may be more expensive.
======

So these are the problems as I see them and potential solutions/workarounds. If there are concerns you have that are not covered here or if you think the solutions won't work for whatever reason, please let me know.

> and also violates a bunch of other reasonable expectations, including the
> concept that the layout after DOM or style mutations is the same layout
> you'd get loading the resulting DOM as a static page.

In particular I don't think this expectation of static/dynamic layout being different should ever be violated, so if there's still some way that might happen please do let me know.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Problem the third: Varying inflation amounts
> --------------------------------------------
> Thus far I have been making a simplifying assumption, which is that for each
> element in a "group" (as per my patch), the element is either not inflated
> at all, or is inflated to the same amount. This assumption is false, and I
> think what you were referring to comment #17. That is, we could have two
> elements A and B that share the same nsRuleNode, but whose actual inflation
> value depends on other factors like the parent frames, and so are inflated
> differently. My current patch will simply pick the inflation amount from the
> first encountered frame and apply this to all of them. This can result in
> non-deterministic behaviour from the web developer's point of view because
> reordering pieces of the page will change how much things get inflated.
> 
> The first option to dealing with this problem is to not use my algorithm in
> this case. That is, if we detect a "group" of elements where there are two
> or more elements with different inflation amounts, we disband the group and
> don't do any copying of inflation. This could be implemented using the same
> nsRuleNode ->* element mapping described above, and by clearing the mapping
> upon disbanding the group.
> 
> Another option is to use a different operator when copying inflation
> amounts. For example, we could take the different inflation amounts of all
> the elements in the group, and use the max() of all of them. Or perhaps the
> average(). (You could say my WIP uses first()). This should solve the
> problem for the web developer in that rearranging the elements doesn't
> change behaviour. Adding/removing elements may still change behaviour
> though. Implementing this may require reflowing all of the elements in a
> group any time any of the elements is modified, and so may be more expensive.

Actually, maybe storing the size (which is a function of the width) is the wrong thing:  maybe all you want to store is whether you've at some point found an inflation container with enough text to enable font inflation.  I think that's likely to produce better behavior anyway; storing the width seems wrong.

> In particular I don't think this expectation of static/dynamic layout being
> different should ever be violated, so if there's still some way that might
> happen please do let me know.

It's the same problem as the ordering (first problem):  you're not actually taking the first in content order, you're taking the first in temporal order.  If a new node is later inserted before the old first node, you won't pick up it's settings.


More on the first and second problems later.

Comment 21

[Scott Johnson (:jwir3)]

Comment on attachment 702458 [details] [diff] [review]
WIP that works

Clearing the feedback? flag, as I think some issues need to be resolved to dbaron's satisfaction before we can land this. (See comment 17 and comment 19).

Comment 23

[Paul [pwd]]

Was the proposed solution truly worse than the current broken implementation? Sorry I have to ask, it's the only way to understand.

Comment 24

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.